Page MenuHomeSoftware Heritage

Modify API output and test
Needs RevisionPublic

Authored by twitu on Jul 10 2019, 5:31 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Summary

In reference to T1433

Changed return type for content_fossology_license_get.
It now returns dict with sha1 key and list of license
and tool information as value.

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
refactor_license_api
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7320
Build 10383: tox-on-jenkinsJenkins
Build 10382: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
twitu updated this revision to Diff 5775.Jul 10 2019, 5:36 PM

small edit and rebase

twitu retitled this revision from Modify output and test to Modify API output and test.Jul 10 2019, 5:39 PM
vlorentz requested changes to this revision.Jul 11 2019, 3:16 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/indexer/storage/__init__.py
445

Use @db_transaction() instead

swh/indexer/storage/in_memory.py
438–442

Good use of defaultdict, thanks

This revision now requires changes to proceed.Jul 11 2019, 3:16 PM
twitu updated this revision to Diff 5807.Jul 11 2019, 8:12 PM

Use db_transaction annotation

twitu updated this revision to Diff 5808.Jul 11 2019, 8:24 PM
  • Modify output and test
  • In reference to T1433
  • In reference to T1433
  • Use db_transaction annotation
vlorentz requested changes to this revision.Jul 18 2019, 11:20 AM
This revision now requires changes to proceed.Jul 18 2019, 11:20 AM
twitu updated this revision to Diff 5919.Jul 21 2019, 10:44 PM
  • Fix test
twitu updated this revision to Diff 5921.Jul 22 2019, 3:53 PM
  • Change expected result
vlorentz requested changes to this revision.Aug 5 2019, 1:49 PM

Do you want help with your diff?

This revision now requires changes to proceed.Aug 5 2019, 1:49 PM
twitu added a comment.Aug 6 2019, 4:30 AM

I haven't been able to give enough time to it. I'll complete the diff by this weekend.

twitu updated this revision to Diff 6193.Aug 11 2019, 4:26 PM
  • Change tests and expected results
twitu added a comment.Aug 12 2019, 5:50 AM

It appears that two different tests TestFossologyLicenseRangeIndexer and TestMimetypeRangeIndexer are inheriting from the same class namely, CommonContentIndexerRangeTest as a result both use the same assert_results_ok method. Both test classes have the same structure for expected results, hence the test works for both, as I experimented in the previous commit, changing the test for the new format of TestFossologyLicenseRangeIndexer will fail for TestMimetypeRangeIndexer. How do I resolve this? Should I override the method in both classes?

The case for CommonContentIndexerTest is similar as it is inherited by TestCtagsIndexer, TestFossologyIndexer and TestMimeTypeIndexer.

In D1720#42747, @twitu wrote:

How do I resolve this? Should I override the method in both classes?

Only override it in TestFossologyLicenseRangeIndexer.

twitu updated this revision to Diff 6205.Aug 12 2019, 5:22 PM
  • Override test methods
twitu updated this revision to Diff 6206.Aug 12 2019, 5:28 PM
  • Override test methods
twitu updated this revision to Diff 6218.Aug 13 2019, 9:59 PM
  • Fix return value for license indexer
twitu updated this revision to Diff 6219.Aug 13 2019, 10:24 PM
  • Fix return value for license indexer
twitu added a comment.EditedAug 13 2019, 10:42 PM

Overiding just the test method doesn't work for TestFossologyLicenseRangeIndexer. Tests test__index_contents and test__index_contents_with_indexed_data call the function self.indexer._index_contents which is implemented in ContentRangeIndexer.

It calls a function index which is not implemented and it yields value when the docstring says it should return a dictionary. I am not sure how to resolve this.

twitu updated this revision to Diff 6220.Aug 14 2019, 5:27 AM
  • Fix return value for license indexer
twitu added a comment.Aug 14 2019, 5:36 AM
In D1720#42908, @twitu wrote:

Overiding just the test method doesn't work for TestFossologyLicenseRangeIndexer. Tests test__index_contents and test__index_contents_with_indexed_data call the function self.indexer._index_contents which is implemented in ContentRangeIndexer.
It calls a function index which is not implemented and it yields value when the docstring says it should return a dictionary. I am not sure how to resolve this.

Keeping the expected results unchanged allows the build to pass, even though it is in the older format. However, as I have quoted, I am not sure how the actual results for the range is being generated by _index_contents. Should I write a new method that intercepts the return value of _index_contents and converts it to the required format? However is such a change within the ambit of this diff?

Keeping the expected results unchanged allows the build to pass, even though it is in the older format.

Is it?

swh/indexer/tests/test_fossology_license.py
125–127

This conditional is not needed

129–130

Neither is this one

133–134

I'm not sure this one is either

136–138

You can replace this for loop with simply: self.assertEqual(expected_results, actual_results).
This will also check there is no extra/unwanted items in actual_results.

vlorentz requested changes to this revision.Aug 19 2019, 11:32 AM
This revision now requires changes to proceed.Aug 19 2019, 11:32 AM
twitu updated this revision to Diff 6281.Aug 20 2019, 6:11 AM
  • Remove unneeded checks
twitu updated this revision to Diff 6282.Aug 20 2019, 6:18 AM
twitu marked 4 inline comments as done.
  • Remove unneeded checks
twitu updated this revision to Diff 6283.Aug 20 2019, 6:33 AM
  • Remove unneeded checks
twitu updated this revision to Diff 6284.Aug 20 2019, 6:49 AM
  • Remove unneeded checks
vlorentz requested changes to this revision.Sep 19 2019, 5:30 PM

@twitu Hi, any news on this?

This revision now requires changes to proceed.Sep 19 2019, 5:30 PM
twitu added a comment.Sep 22 2019, 1:00 PM

I am sorry for the delay. Due to some dependency issue, I am unable to run the tests locally which is why I have been making so many pushes and am unable to solve it effectively. I will reinstall all the whole environment and try to fix it, if the environment works I will complete this. If I am unable to fix my environment then sadly I will have to drop this one.

Did you try tox --recreate?

twitu added a comment.Sep 22 2019, 1:40 PM

The problem is with the python setup something to do with typing.py giving errors if I remember correctly. I will try it again and add a error log.