Skip to content

BF: test_addurls: Avoid modifying shared test data#5674

Merged
kyleam merged 1 commit intodatalad:maintfrom
kyleam:test-addurls-copy-fix
May 19, 2021
Merged

BF: test_addurls: Avoid modifying shared test data#5674
kyleam merged 1 commit intodatalad:maintfrom
kyleam:test-addurls-copy-fix

Conversation

@kyleam
Copy link
Copy Markdown
Contributor

@kyleam kyleam commented May 19, 2021

test_addurls_row_missing_key_fields() makes a shallow copy of the
data attribute (list of dictionaries) exposed to all TestAddurls
tests and then modifies one of the dictionaries to remove a key, which
of course modifies the row in TestAddurls.data.

Make a deep copy to avoid interfering with other tests.


As far as I know, this doesn't trigger any failures right now, but it will in an upcoming PR (against master).

@codecov
Copy link
Copy Markdown

codecov bot commented May 19, 2021

Codecov Report

Merging #5674 (c67b55c) into maint (e8ea043) will decrease coverage by 5.28%.
The diff coverage is 100.00%.

❗ Current head c67b55c differs from pull request most recent head 1bad217. Consider uploading reports for the commit 1bad217 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5674      +/-   ##
==========================================
- Coverage   90.30%   85.01%   -5.29%     
==========================================
  Files         299      296       -3     
  Lines       42348    42331      -17     
==========================================
- Hits        38242    35989    -2253     
- Misses       4106     6342    +2236     
Impacted Files Coverage Δ
datalad/plugin/tests/test_addurls.py 99.54% <100.00%> (+<0.01%) ⬆️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
datalad/metadata/extractors/exif.py 18.75% <0.00%> (-78.13%) ⬇️
datalad/metadata/extractors/image.py 19.35% <0.00%> (-77.42%) ⬇️
datalad/metadata/extractors/audio.py 20.00% <0.00%> (-77.15%) ⬇️
datalad/metadata/extractors/tests/test_exif.py 24.00% <0.00%> (-76.00%) ⬇️
datalad/metadata/extractors/tests/test_xmp.py 26.92% <0.00%> (-73.08%) ⬇️
datalad/metadata/extractors/tests/test_image.py 26.92% <0.00%> (-73.08%) ⬇️
... and 77 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8ea043...1bad217. Read the comment docs.

test_addurls_row_missing_key_fields() makes a shallow copy of the
`data` attribute (list of dictionaries) exposed to all TestAddurls
tests and then modifies one of the dictionaries to remove a key, which
of course modifies the row in TestAddurls.data.

Make a deep copy to avoid interfering with other tests.
@kyleam kyleam force-pushed the test-addurls-copy-fix branch from a377a29 to 1bad217 Compare May 19, 2021 19:17
@kyleam
Copy link
Copy Markdown
Contributor Author

kyleam commented May 19, 2021

I think this one's uncontroversial. Will merge to unblock gh-5675

@kyleam kyleam merged commit d7a5928 into datalad:maint May 19, 2021
Copy link
Copy Markdown
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds and looks good to me. I don't think we would run into a use case where deepcopy might become memory prohibitive. (was too fast to enter without realizing we are talking about a test ;))

Thank you!

@kyleam kyleam deleted the test-addurls-copy-fix branch May 19, 2021 21:54
@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2021
@yarikoptic yarikoptic mentioned this pull request Jun 3, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants