Page MenuHomeSoftware Heritage

origin_save: Add docstring and some test scenarios
ClosedPublic

Authored by ardumont on Apr 19 2021, 6:00 PM.

Details

Summary

In trying to reproduce the error i've seen in T3266 and failing to reproduce it. I have
added some more docstrings and some more test scenarios around the save code now.

Related to T3266

Test Plan

tox

Diff Detail

Repository
rDWAPPS Web applications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20855
Build 32362: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32361: arc lint + arc unit

Event Timeline

swh/web/tests/common/test_origin_save.py
240

note that this is confusing when one wants to override the date [1]
The model is already auto adding the date.

That means we can remove that line and let the default django behavior occur ;)

[1] I tried locally at some point to try and push some date beyong the threshold. I did
not push it because I still failed to reproduce the issue in tests.

307

because that's what the code is doing, it overwrite the visit_date to none for that case.

338

That test is confusing but so is the current behavior so status quo ;)

Build is green

Patch application report for D5554 (id=19846)

Rebasing onto f3f1b7c84a...

Current branch diff-target is up to date.
Changes applied before test
commit d31ab3d188b567187f695dbb30d7513c77634657
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 17:56:06 2021 +0200

    origin_save: Add docstring and some test scenarios
    
    Related to T3266

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/691/ for more details.

Build is green

Patch application report for D5554 (id=19852)

Rebasing onto c908051ca7...

Current branch diff-target is up to date.
Changes applied before test
commit 4a6d884a7449bf01c1ce8fda24ab6286fb887129
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Apr 19 17:56:06 2021 +0200

    origin_save: Add docstring and some test scenarios
    
    Related to T3266

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/695/ for more details.

Can you use exhaustive comparisons instead of len(sors) == 1 + comparing items?

swh/web/tests/common/test_origin_save.py
264

I don't think this deserves a comment

Can you use exhaustive comparisons instead of len(sors) == 1 + comparing items?

Ok, i'll check.
I just adapted according to how that was done.

But right, might as well improve along the way.

swh/web/tests/common/test_origin_save.py
264

i'm pretty sure you might have asked me why the change without the comment ;)

Can you use exhaustive comparisons instead of len(sors) == 1 + comparing items?

We could but in another diff.

Looks good to me otherwise.

swh/web/tests/common/test_origin_save.py
240

Right, I guess you can remove that line in a separate commit.

264

agreed

307

Indeed, and it should not do it, that's a mistake in the save code now code.

As a visit has been made, the date should not be discarded.

To investigate later.

338

Because the behavior prior exploiting visit statuses was to check scheduler task statuses (because only that info was available at the time).

I guess we could mark a save request as succeeded only if a visit status has been found now (in another diff).

This revision is now accepted and ready to land.Apr 20 2021, 11:38 AM

We could but in another diff.

ok ;)

swh/web/tests/common/test_origin_save.py
338

yes, quite.

I planned to update the behavior and modify some more those tests accordingly
to make this more apparent (not that it was not yet clear exactly how though ;)