-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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
Add Firefox major version support (#668) #675
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR as well! Before I'm going to actually review would you mind to add some tests that verify the changes? Local tests will be enough but maybe we might have to add some more test data to get this properly tested. Thanks.
Test cases and test data added @whimboo |
tests/data/firefox/releases/23.0/win32/en-US/Firefox Setup 23.0.exe
Outdated
Show resolved
Hide resolved
… case with loren ipsum
…ase test case with 23.1.0
…0 version candidates
…idate test cases with 23.1.0
Added 23.1.0 test data, add test cases @whimboo |
({'platform': 'win32', 'version': '23'}, | ||
'firefox-23.1.0.en-US.win32.exe', | ||
'firefox/releases/23.1.0/win32/en-US/Firefox Setup 23.1.0.exe'), | ||
({'platform': 'win32', 'version': '23.0'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, for this specific test I would actually expect that the 23.0.1 release would be downloaded which is the latest security release of the 23.0 minor version. And we have this release already present for the testing data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this change because I added 23.1.0 test data to the folder, so 23 ==> 23.1.0 instead of 23.0.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the test doesn't specify 23
but is more strict in requesting a version for 23.0
. That means that we cannot allow a download of 23.1.x
but have to stick to the minor version which is 23.0.x
and as such we still need to return 23.0.1
.
Hi @piri-p. I wanted to check back with you and ask if you may have the time to finish this particular PR. Thanks. |
Hi @piri-p. If you aren't able to complete this PR would it be fine if someone else finishes it up? Thanks! |
Fix #668 - Add Firefox major version support @whimboo