Page MenuHomeSoftware Heritage

Modify API output and test
Needs ReviewPublic

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

twitu created this revision.Jul 10 2019, 5:31 PM
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.Mon, Jul 22, 3:53 PM
  • Change expected result
vlorentz requested changes to this revision.Mon, Aug 5, 1:49 PM

Do you want help with your diff?

This revision now requires changes to proceed.Mon, Aug 5, 1:49 PM
twitu added a comment.Tue, Aug 6, 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.Sun, Aug 11, 4:26 PM
  • Change tests and expected results
twitu added a comment.Mon, Aug 12, 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.Mon, Aug 12, 5:22 PM
  • Override test methods
twitu updated this revision to Diff 6206.Mon, Aug 12, 5:28 PM
  • Override test methods
twitu updated this revision to Diff 6218.Tue, Aug 13, 9:59 PM
  • Fix return value for license indexer
twitu updated this revision to Diff 6219.Tue, Aug 13, 10:24 PM
  • Fix return value for license indexer
twitu added a comment.EditedTue, Aug 13, 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.Wed, Aug 14, 5:27 AM
  • Fix return value for license indexer
twitu added a comment.Wed, Aug 14, 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.Mon, Aug 19, 11:32 AM
This revision now requires changes to proceed.Mon, Aug 19, 11:32 AM
twitu updated this revision to Diff 6281.Tue, Aug 20, 6:11 AM
  • Remove unneeded checks
twitu updated this revision to Diff 6282.Tue, Aug 20, 6:18 AM
twitu marked 4 inline comments as done.
  • Remove unneeded checks
twitu updated this revision to Diff 6283.Tue, Aug 20, 6:33 AM
  • Remove unneeded checks
twitu updated this revision to Diff 6284.Tue, Aug 20, 6:49 AM
  • Remove unneeded checks