-
Notifications
You must be signed in to change notification settings - Fork 3k
Add unit test for scancode-evaluate.py and improve analysis of scancode results #13703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@harmut01, thank you for your changes. |
ba7d8a4
to
aaad882
Compare
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.
c6d15c5
to
3c6c8ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace all tabs in tools/test/travis-ci/scancode_evaluate_test.py
with spaces.
""" | ||
found_spdx = False | ||
# iterate through licenses, stop once permissive license has been found | ||
for i in range(len(license_offender['file']['licenses'])): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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':
...
else: | ||
file.write(invalid_header_1) | ||
|
||
expected_result = 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the docstring to reflect this - I hope it makes sense...
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.
3c6c8ae
to
5753ed9
Compare
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.
07639ce
to
2752914
Compare
os.remove(stub_path + "test5.h") | ||
|
||
def test_scancode_case_4(self): | ||
""" Test Case 4 -- license header permissive and non-permissive 'license' [FP] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is [FP]
?
|
||
result = license_check(ROOT, test_json) | ||
|
||
self.assertEqual(expected_result, result, "Non-Permissive Header False Positive") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use os.path
or pathlib
instead of raw strings for paths, this ensures cross-platform compatibility.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import json | ||
from unittest import TestCase | ||
|
||
# TODO: fix scancode to match python naming conventROOTi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix "conventROOTi" typo.
) | ||
|
||
# path to stub files | ||
stub_path = ROOT + "/scancode_test/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stub_path = ROOT + "/scancode_test/" | |
stub_path = os.path.join(ROOT, "scancode_test") |
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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define some named constants for the return codes so it's easier to understand what "-1" or "7" means?
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Replaced by: #13745 |
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
Impact of changes
Impacts CI testing
Migration actions required
None
Documentation
None
Pull request type
Test results
Reviewers
@hugueskamba @ARMmbed/mbed-os-core