Add unit test for scancode-evaluate.py and improve analysis of scancode results #13703

harmut01 · 2020-10-02T15:55:12Z

Summary of changes

Fixes bug in scancode-evaluate.py where files were being incorrectly labelled as non-permissively licensed. Also added unit testing files:

scancode_evaluate_test.py
folder for stub files 'scancode_test/'
stub files: 4 JSON, 4 source files

Impact of changes

Impacts CI testing

Migration actions required

None

Documentation

None

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@hugueskamba @ARMmbed/mbed-os-core

ciarmcom · 2020-10-02T16:00:37Z

@harmut01, thank you for your changes.
@hugueskamba @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

Add unit test and dependent stubs for testing of scancode-evaluate.py. license_check takes a JSON as argument and checks for source missing copyright or license notices. Each JSON represents a separate test case, and stubs are added for test case 3 and 4 where license_check looks for an spdx notice in the source file.

hugueskamba

Please replace all tabs in tools/test/travis-ci/scancode_evaluate_test.py with spaces.

tools/test/travis-ci/scancode_evaluate_test.py

tools/test/travis-ci/scancode_test/scancode_test_1.json

rwalton-arm · 2020-10-07T14:06:22Z

tools/test/travis-ci/scancode-evaluate.py

+        """
+        found_spdx = False
+        # iterate through licenses, stop once permissive license has been found
+        for i in range(len(license_offender['file']['licenses'])):


The for statement will automatically iterate over elements of a sequence. So, instead of explicitly calling range(len(... you can just have something like:

for license in license_offender['file']['licenses']: if license['category'] == 'Permissive': ...

hugueskamba · 2020-10-07T18:12:24Z

tools/test/travis-ci/scancode_evaluate_test.py

+				else:
+					file.write(invalid_header_1)
+
+		expected_result = 7


Is this correct? You expect 7 but the method docstring says it should be 5 and the scancode_test_3.json has 5 files. Can you explain the 7? What are the 7 errors?

Apologies, I forgot to update the docstring! My understanding is that we want to be able to address the errors in the file individually. For this reason two of the stubs contain a non-permissive license and no SPDX to test this. In total 4 errors are attributed to these two files (2 non-permissive, 2 SPDX missing).

I've updated the docstring to reflect this - I hope it makes sense...

hugueskamba · 2020-10-07T20:13:17Z

Can you explain in the description why the file was incorrectly labelled as non-permissive?

Describe in the description the condition you are looking for to determine whether a file has a permissive license or not.

Add unit test and dependent stubs for testing of scancode-evaluate.py. license_check takes a JSON as argument and checks for source missing copyright or license notices. Each JSON represents a separate test case, and stubs are added for test case 3 and 4 where license_check looks for an spdx notice in the source file.

Pull request has been modified.

Add unit test and dependent stubs for testing of scancode-evaluate.py. license_check takes a JSON as argument and checks for source missing copyright or license notices. Each JSON represents a separate test case, and stubs are added for test case 3 and 4 where license_check looks for an spdx notice in the source file.

hugueskamba · 2020-10-08T00:01:45Z

tools/test/travis-ci/scancode_evaluate_test.py

+		os.remove(stub_path + "test5.h")
+
+	def test_scancode_case_4(self):
+		""" Test Case 4 -- license header permissive and non-permissive 'license' [FP]


What is [FP]?

hugueskamba · 2020-10-08T00:03:53Z

tools/test/travis-ci/scancode_evaluate_test.py

+
+		result = license_check(ROOT, test_json)
+
+		self.assertEqual(expected_result, result, "Non-Permissive Header False Positive")


How can you expect it to pass if scancode_test_4.json has one license missing spdx in matched_rule.identifier? Am I missing something?

rwalton-arm · 2020-10-08T15:38:49Z

tools/test/travis-ci/scancode_evaluate_test.py

+            @outputs -1 if any error in file licenses found
+        """
+        # generate scancode_test_1.json
+        with open(ROOT + "/scancode_test/scancode_test_1.json", 'w') as f:


Use os.path or pathlib instead of raw strings for paths, this ensures cross-platform compatibility.

rwalton-arm · 2020-10-08T15:42:06Z

tools/test/travis-ci/scancode_evaluate_test.py

+            json.dump({"headers": {"tool name": "scancode test fail"}}, f)
+
+        self.assertEqual(-1, license_check(ROOT, ROOT + "/scancode_test/scancode_test_1.json"))
+        os.remove(ROOT + "/scancode_test/scancode_test_1.json")


If an exception is raised and the program exits before we get here, the files will not be removed. Consider using tempfile.TemporaryDirectory as a context manager to ensure clean-up happens in an exception-safe way.

rwalton-arm · 2020-10-08T15:44:32Z

tools/test/travis-ci/scancode_evaluate_test.py

+import json
+from unittest import TestCase
+
+# TODO: fix scancode to match python naming conventROOTi


Please fix "conventROOTi" typo.

rwalton-arm · 2020-10-08T15:45:25Z

tools/test/travis-ci/scancode_evaluate_test.py

+)
+
+# path to stub files
+stub_path = ROOT + "/scancode_test/"


Suggested change

stub_path = ROOT + "/scancode_test/"

stub_path = os.path.join(ROOT, "scancode_test")

rwalton-arm · 2020-10-08T15:47:33Z

tools/test/travis-ci/scancode_evaluate_test.py

+        with open(ROOT + "/scancode_test/scancode_test_1.json", 'w') as f:
+            json.dump({"headers": {"tool name": "scancode test fail"}}, f)
+
+        self.assertEqual(-1, license_check(ROOT, ROOT + "/scancode_test/scancode_test_1.json"))


Can we define some named constants for the return codes so it's easier to understand what "-1" or "7" means?

rwalton-arm · 2020-10-08T15:49:19Z

tools/test/travis-ci/scancode_evaluate_test.py

+        self.assertEqual(-1, license_check(ROOT, ROOT + "/scancode_test/scancode_test_1.json"))
+        os.remove(ROOT + "/scancode_test/scancode_test_1.json")
+
+    def test_scancode_case_2(self):


Suggested change

def test_scancode_case_2(self):

def test_scancode_no_errors_in_license_headers(self):

test_scancode_case_2 is not a useful test case name. Same comment for other test cases.

hugueskamba · 2020-10-09T08:24:32Z

Replaced by: #13745

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Oct 2, 2020

ciarmcom requested review from hugueskamba and a team October 2, 2020 16:00

ciarmcom added the needs: review label Oct 2, 2020

mergify bot added needs: work and removed needs: review labels Oct 2, 2020

harmut01 force-pushed the evaluate_code_fix branch from ba7d8a4 to aaad882 Compare October 5, 2020 10:27

harmut01 changed the title ~~fix for bug in scancode-evaluate.py that caused incorrect labelling o…~~ Add unit test for scancode-evaluate.py and improve analysis of scancode results Oct 5, 2020

harmut01 force-pushed the evaluate_code_fix branch 6 times, most recently from c6d15c5 to 3c6c8ae Compare October 6, 2020 17:19

hugueskamba reviewed Oct 7, 2020

View reviewed changes

hugueskamba previously requested changes Oct 7, 2020

View reviewed changes

rwalton-arm reviewed Oct 7, 2020

View reviewed changes

hugueskamba reviewed Oct 7, 2020

View reviewed changes

harmut01 force-pushed the evaluate_code_fix branch from 3c6c8ae to 5753ed9 Compare October 8, 2020 08:52

harmut01 force-pushed the evaluate_code_fix branch from 07639ce to 2752914 Compare October 8, 2020 10:37

hugueskamba reviewed Oct 8, 2020

View reviewed changes

rwalton-arm reviewed Oct 8, 2020

View reviewed changes

hugueskamba closed this Oct 9, 2020

mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch labels Oct 9, 2020


		result = license_check(ROOT, test_json)

		self.assertEqual(expected_result, result, "Non-Permissive Header False Positive")

	stub_path = ROOT + "/scancode_test/"
	stub_path = os.path.join(ROOT, "scancode_test")

	def test_scancode_case_2(self):
	def test_scancode_no_errors_in_license_headers(self):

Add unit test for scancode-evaluate.py and improve analysis of scancode results #13703

Add unit test for scancode-evaluate.py and improve analysis of scancode results #13703

Uh oh!

Conversation

harmut01 commented Oct 2, 2020 • edited by hugueskamba Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Oct 2, 2020

Uh oh!

hugueskamba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwalton-arm Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented Oct 9, 2020

Uh oh!

Uh oh!

harmut01 commented Oct 2, 2020 •

edited by hugueskamba

Loading

rwalton-arm Oct 7, 2020 •

edited

Loading

hugueskamba Oct 7, 2020 •

edited

Loading

hugueskamba commented Oct 7, 2020 •

edited

Loading