Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

harmut01
Copy link
Contributor

@harmut01 harmut01 commented Oct 2, 2020

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 ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Oct 2, 2020
@ciarmcom
Copy link
Member

ciarmcom commented Oct 2, 2020

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

@ciarmcom ciarmcom requested review from hugueskamba and a team October 2, 2020 16:00
@harmut01 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
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.
@harmut01 harmut01 force-pushed the evaluate_code_fix branch 6 times, most recently from c6d15c5 to 3c6c8ae Compare October 6, 2020 17:19
Copy link
Collaborator

@hugueskamba hugueskamba left a 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'])):
Copy link
Contributor

@rwalton-arm rwalton-arm Oct 7, 2020

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
Copy link
Collaborator

@hugueskamba hugueskamba Oct 7, 2020

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?

Copy link
Contributor Author

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).

Copy link
Contributor Author

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...

@hugueskamba
Copy link
Collaborator

hugueskamba commented Oct 7, 2020

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.
@mergify mergify bot dismissed hugueskamba’s stale review October 8, 2020 08:52

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.
os.remove(stub_path + "test5.h")

def test_scancode_case_4(self):
""" Test Case 4 -- license header permissive and non-permissive 'license' [FP]
Copy link
Collaborator

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")
Copy link
Collaborator

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:
Copy link
Contributor

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")
Copy link
Contributor

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
Copy link
Contributor

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/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"))
Copy link
Contributor

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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Replaced by: #13745

@hugueskamba hugueskamba closed this Oct 9, 2020
@mergify mergify bot removed needs: work release-type: patch Indentifies a PR as containing just a patch labels Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants