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

Fix an issue with invalid build number for candidate scraper #607

Merged
merged 4 commits into from
Aug 14, 2020
Merged

Fix an issue with invalid build number for candidate scraper #607

merged 4 commits into from
Aug 14, 2020

Conversation

ashellunts
Copy link
Contributor

Now scraper fails when invalid build number is used instead of downloading other build

Fixes issue #445

@ashellunts
Fix an issue with invalid build number for candidate scraper
8881c7a 
Now scraper fails when invalid build number is used instead of downloading other build

Fixes issue #445
@ashellunts
Fix devedition tests
631d46a 
Fixed build numbers in devedition tests
@coveralls
Copy link

coveralls commented Jun 14, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 92.898% when pulling 9e98c06 on ashellunts:invalid_build_number into d62c75b on mozilla:master.

@ashellunts
Copy link
Contributor Author

Hi @whimboo, could you review please?

@ashellunts
Copy link
Contributor Author

Hi @whimboo, friendly reminder :)

@whimboo
Copy link
Contributor

whimboo commented Jul 9, 2020

Sorry for the delay. I was kinda swamped with other work the last weeks. I will try my best to get to the review tomorrow.

@ashellunts
Copy link
Contributor Author

No worries, thank you.

whimboo
whimboo requested changes Jul 19, 2020
View reviewed changes
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Thanks for your patch, and sorry again for the long delay in review. The patch in general works fine, but there are some inline comments I would like to see explained/fixed before we can get it landed.

tests/remote/test_devedition.py
@@ -39,18 +39,18 @@ def test_release_scraper(tmpdir, args, url):

@pytest.mark.ci_only
@pytest.mark.parametrize("args,url", [
({'application': 'devedition', 'platform': 'linux', 'version': '60.0b1', 'build_number': 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain, why you've made these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every test case here has build 3 in its URL, for example:

devedition/candidates/60.0b1-candidates/build3/linux-i686/en-US/firefox-60.0b1.tar.bz2

Before the fix, if build with requested build_number was not found, then first build was used. So these tests started failing after the fix because build_number 1 was used as an argument.

So I have fixed tests to use build_number 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! What an oversight when we created those tests. Good that we cover those cases now. Thanks for explaining.

tests/release_candidate_scraper/test_release_candidate_scraper_indices.py Outdated
args = {'application': 'firefox', 'build_number': '2', 'platform': 'linux', 'version': '23.0.1'}

with pytest.raises(errors.NotSupportedError):
ReleaseCandidateScraper(destination=tmpdir, base_url=httpd.get_url(), **args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a new line at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

mozdownload/scraper.py Show resolved Hide resolved
mozdownload/scraper.py Outdated
self.logger.info('Selected build: %s' %
(parser.entries[self.build_index]))
message = 'Selected build not available!'
self.logger.info(message)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to log this extra line. Immediately raise the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

tests/release_candidate_scraper/test_release_candidate_scraper_indices.py Outdated
@@ -6,22 +6,29 @@

import pytest

from mozdownload import ReleaseCandidateScraper
from mozdownload import ReleaseCandidateScraper, errors
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to keep the alphabetical order of imports. So please exchange both entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

ashellunts
ashellunts commented Jul 19, 2020
View reviewed changes
Copy link
Contributor Author

@ashellunts ashellunts left a comment

Choose a reason for hiding this comment

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

Thanks for your patch, and sorry again for the long delay in review. The patch in general works fine, but there are some inline comments I would like to see explained/fixed before we can get it landed.

Thank you for comments.

tests/release_candidate_scraper/test_release_candidate_scraper_indices.py Outdated
args = {'application': 'firefox', 'build_number': '2', 'platform': 'linux', 'version': '23.0.1'}

with pytest.raises(errors.NotSupportedError):
ReleaseCandidateScraper(destination=tmpdir, base_url=httpd.get_url(), **args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@ashellunts ashellunts requested a review from whimboo July 19, 2020 22:03
@ashellunts
Remove unnecessary logs from get_build_info
e267815
@ashellunts
Fix order of imports
9e98c06 
And added new line in the end of test_release_candidate_scraper_indices.py
@ashellunts
Copy link
Contributor Author

Friendly reminder :)

@whimboo
Copy link
Contributor

whimboo commented Aug 14, 2020

Sorry, but I was on PTO for a while. I will get back as soon as possible.

whimboo
whimboo approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work and I'm very sorry for all the delays in reviewing this PR.

tests/remote/test_devedition.py
@@ -39,18 +39,18 @@ def test_release_scraper(tmpdir, args, url):

@pytest.mark.ci_only
@pytest.mark.parametrize("args,url", [
({'application': 'devedition', 'platform': 'linux', 'version': '60.0b1', 'build_number': 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! What an oversight when we created those tests. Good that we cover those cases now. Thanks for explaining.

@whimboo whimboo merged commit 4596d22 into mozilla:master Aug 14, 2020
@whimboo whimboo mentioned this pull request Aug 14, 2020
Closed
@ashellunts
Copy link
Contributor Author

Thanks a lot for your work and I'm very sorry for all the delays in reviewing this PR.

No problem. Thank you for accepting changes.

@ashellunts ashellunts deleted the invalid_build_number branch August 15, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whimboo whimboo whimboo 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.
3 participants
@ashellunts @coveralls @whimboo