Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-37439: Fix FileReadWriteTestCase test #44

Merged
merged 2 commits into from Mar 24, 2023
Merged

DM-37439: Fix FileReadWriteTestCase test #44

merged 2 commits into from Mar 24, 2023

Conversation

Fireye04
Copy link
Member

@Fireye04 Fireye04 commented Mar 3, 2023 •
edited

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@codecov
Copy link

codecov bot commented Mar 3, 2023 •
edited

Codecov Report

Patch coverage: 68.75% and project coverage change: -0.07 ⚠️

Comparison is base (82ed195) 84.54% compared to head (e68c2e6) 84.47%.

❗ Current head e68c2e6 differs from pull request most recent head dbce7c8. Consider uploading reports for the commit dbce7c8 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
- Coverage   84.54%   84.47%   -0.07%     
==========================================
  Files          27       27              
  Lines        3649     3665      +16     
  Branches      764      767       +3     
==========================================
+ Hits         3085     3096      +11     
- Misses        439      441       +2     
- Partials      125      128       +3
Impacted Files Coverage Δ
python/lsst/resources/s3utils.py 60.37% <58.33%> (-0.27%) ⬇️
tests/test_s3.py 87.65% <100.00%> (+0.31%) ⬆️
tests/test_s3utils.py 78.43% <100.00%> (+0.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

timj
timj reviewed Mar 3, 2023
View reviewed changes
tests/test_s3.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 force-pushed the tickets/DM-37439 branch 2 times, most recently from 591ac66 to d44b304 Compare March 3, 2023 21:31
@Fireye04 Fireye04 requested a review from timj March 3, 2023 21:34
@Fireye04 Fireye04 force-pushed the tickets/DM-37439 branch 4 times, most recently from df8304f to 003539c Compare March 3, 2023 23:31
timj
timj requested changes Mar 3, 2023
View reviewed changes
python/lsst/resources/tests.py Outdated Show resolved Hide resolved
tests/test_s3.py Outdated Show resolved Hide resolved
tests/test_s3.py Outdated Show resolved Hide resolved
python/lsst/resources/tests.py Outdated Show resolved Hide resolved
@Fireye04 Fireye04 force-pushed the tickets/DM-37439 branch 4 times, most recently from d5c9ade to 0dbdd53 Compare March 3, 2023 23:57
@Fireye04 Fireye04 requested a review from timj March 4, 2023 00:03
timj
timj approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks good. Needs some minor fixes.

python/lsst/resources/s3utils.py Outdated Show resolved Hide resolved
python/lsst/resources/s3utils.py Show resolved Hide resolved
python/lsst/resources/tests.py Outdated Show resolved Hide resolved
tests/test_s3.py Show resolved Hide resolved
timj
timj requested changes Mar 7, 2023
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I hit the wrong button last time 😄

@Fireye04 Fireye04 force-pushed the tickets/DM-37439 branch 4 times, most recently from 9146cde to b8f9b96 Compare March 24, 2023 17:44
@Fireye04 Fireye04 requested a review from timj March 24, 2023 17:45
@Fireye04
Fix FileReadWriteTestCase test
7320635 
Attempted to use patch.dict, however it did not work for reasons unknown
timj
timj approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks okay. I've made a couple of suggestions to the docstring.

python/lsst/resources/s3utils.py Outdated Show resolved Hide resolved
python/lsst/resources/s3utils.py Outdated Show resolved Hide resolved
@Fireye04 @timj
Fix s3utils errors
dbce7c8 
Add docstring to the clean_test_environment function

Co-authored-by: Tim Jenness <tjenness@lsst.org>
@Fireye04 Fireye04 merged commit c35d886 into main Mar 24, 2023
13 checks passed
@Fireye04 Fireye04 deleted the tickets/DM-37439 branch March 24, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timj timj timj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Fireye04 @timj