Page MenuHomeSoftware Heritage

Modify API output and test
AbandonedPublic

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

Details

Reviewers
twitu
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 6848
Build 9584: tox-on-jenkinsJenkins
Build 9583: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
twitu retitled this revision from Modify output and test to Modify API output and test.Jul 10 2019, 5:39 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

Use db_transaction annotation

  • Modify output and test
  • In reference to T1433
  • In reference to T1433
  • Use db_transaction annotation
This revision now requires changes to proceed.Jul 18 2019, 11:20 AM
  • 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

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

  • Change tests and expected results

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.

  • Fix return value for license indexer
  • Fix return value for license indexer

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.

  • Fix return value for license indexer
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 ↗(On Diff #6220)

This conditional is not needed

129–130 ↗(On Diff #6220)

Neither is this one

133–134 ↗(On Diff #6220)

I'm not sure this one is either

136–138 ↗(On Diff #6220)

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.

This revision now requires changes to proceed.Aug 19 2019, 11:32 AM
  • Remove unneeded checks
twitu marked 4 inline comments as done.
  • Remove unneeded checks
  • Remove unneeded checks
  • Remove unneeded checks

@twitu Hi, any news on this?

This revision now requires changes to proceed.Sep 19 2019, 5:30 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.

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.

We refactored this code, so I'm going to close this diff

vlorentz abandoned this revision.
vlorentz edited reviewers, added: twitu; removed: vlorentz.