Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[tests] Fix failing tests #4

Closed
wants to merge 3 commits into from
Closed

[tests] Fix failing tests #4

wants to merge 3 commits into from

Conversation

inishchith
Copy link
Contributor

@inishchith inishchith commented Feb 23, 2019 •
edited

  • fix links in test files for NOMOS and SCANCODE
  • fix badge links in readme
  • fix .travis.yml for storing executables from scancode and fossology/nomos
  • All test passed

@valeriocos Please review and let me know if any changes required 馃槃

Note: Reference. There's 1 test which still fails due to bad JSON at this line.

closes #3

@inishchith inishchith force-pushed the fix_tests branch 10 times, most recently from f287a39 to e68f5b2 Compare February 24, 2019 07:45
@valeriocos
Copy link
Member

@inishchith thank you a lot for the PR. I cannot test it today, but I'll do it tomorrow.

@inishchith
Copy link
Contributor Author

@valeriocos Sure, No problem. Though i had a concern, I need some suggestion in fixing the last test issue here . I think it's due to bad JSON return.

@valeriocos valeriocos self-requested a review February 25, 2019 09:02
valeriocos
valeriocos reviewed Feb 25, 2019
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
valeriocos
valeriocos reviewed Feb 25, 2019
View reviewed changes
tests/test_colic.py Outdated Show resolved Hide resolved
valeriocos
valeriocos reviewed Feb 25, 2019
View reviewed changes
tests/test_nomos.py Outdated Show resolved Hide resolved
valeriocos
valeriocos reviewed Feb 25, 2019
View reviewed changes
tests/test_scancode.py Outdated Show resolved Hide resolved
@valeriocos
Copy link
Member

Thank @inishchith for the PR, overall LGTM, I left some minor comments.
WRT the failing test, I don't detect the problem locally when installing the release 3.0.0 of scancode, instead the error pops up when cloning the repo.
Please try to change the .travis.yml to install that version of scancode.

@inishchith inishchith force-pushed the fix_tests branch 9 times, most recently from b038d88 to 8091c3d Compare February 25, 2019 14:37
@inishchith
Copy link
Contributor Author

@valeriocos Thanks for reviewing the PR ;)

  • I've added the PATH in utils.py
  • I did try version v3.0.0 and v2.9.2 after your suggestion, but the results are same.
  • Ref v3.0.0
  • Ref v2.9.2

@valeriocos
Copy link
Member

no worries, I would merge the PR anyway and take a closer look later.

If you agree, could you reorganize the commits in the following way, adding also msgs and descrs:

  • 1 commit including the modifications in the tests folder -> [tests] ...
  • 1 commit for the changes in README.md and setup.py -> [doc] ...
  • 1 commits for the changes in .travis.yml -> [travis] ...

thank you @inishchith

@inishchith
[doc] fix urls in Readme.md and setup.py
a372997 
Fix urls for build status, code coverage under Readme.md
Fix url for github repository under setup.py
@inishchith
Copy link
Contributor Author

@valeriocos Thanks for the review.
That looks like a great convention to follow. I've made the changes.
Let me know if any changes required 馃槃 .
Also please do let me know once you take a closer look on the issue, even i'll try and fix it ASAP ;)

@inishchith inishchith force-pushed the fix_tests branch 3 times, most recently from 67eb146 to 1794854 Compare February 26, 2019 05:00
@valeriocos
Copy link
Member

@inishchith overall the PR looks great, I've left just some minor comments.
Thank you for your time.

@inishchith
[tests] Move executable path to utils.py
27500b5 
Move executable paths - NOMOS_PATH and SCANCODE_PATH to utils.py
@inishchith
[travis] fix travis failing tests
4052921 
Add executable of NOMOS_PATH and SCANCODE_PATH before running scripts
@inishchith
Copy link
Contributor Author

@valeriocos Thanks for all the help. The tests pass completely now 馃槄

@valeriocos
Copy link
Member

valeriocos commented Feb 26, 2019 •
edited

Thank you a lot @inishchith for the PR, I have just merged it (I don't know why it looks like it wasn't merged).

@inishchith
Copy link
Contributor Author

@valeriocos Thanks for the merge 馃槃 .
I'm waiting of the Build Passing 馃帀 to appear on the Repository Readme 馃槢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valeriocos valeriocos valeriocos left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ tests ] Fix Failing tests
2 participants
@inishchith @valeriocos