Closed selenium-ci closed 1 month ago
⏱️ Estimated effort to review [1-5] | 2, because the changes are straightforward updates to URLs and checksums for browser versions. The structure and logic of the code remain unchanged, making it relatively easy to review. |
🧪 Relevant tests | No |
⚡ Possible issues | Possible Dependency Issue: Ensure that the updated browser versions are compatible with the existing system and do not introduce new compatibility issues. |
🔒 Security concerns | No |
Category | Suggestion | Score |
Maintainability |
Define version numbers and URLs as variables to improve maintainability___ **To improve maintainability, consider defining the version numbers and URLs as variables atthe top of the file. This will make future updates easier and reduce the risk of errors.** [common/repositories.bzl [53-54]](https://github.com/SeleniumHQ/selenium/pull/14037/files#diff-25d82cd18102fed27d3202000e1f1b3a56a85ad2848236d91989cd30a3952401R53-R54) ```diff -url = "https://ftp.mozilla.org/pub/firefox/releases/127.0b6/linux-x86_64/en-US/firefox-127.0b6.tar.bz2", -sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", +FIREFOX_VERSION = "127.0b6" +FIREFOX_URL = f"https://ftp.mozilla.org/pub/firefox/releases/{FIREFOX_VERSION}/linux-x86_64/en-US/firefox-{FIREFOX_VERSION}.tar.bz2" +FIREFOX_SHA256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3" +url = FIREFOX_URL, +sha256 = FIREFOX_SHA256, + ``` Suggestion importance[1-10]: 7Why: Defining version numbers and URLs as variables at the top of the file is a significant improvement for maintainability and reduces the risk of errors during updates. | 7 |
Enhancement |
Add a retry mechanism for downloading the archives to handle transient network issues___ **Consider adding a retry mechanism for downloading the archives to handle transient networkissues. This can improve the robustness of the build process.** [common/repositories.bzl [53-54]](https://github.com/SeleniumHQ/selenium/pull/14037/files#diff-25d82cd18102fed27d3202000e1f1b3a56a85ad2848236d91989cd30a3952401R53-R54) ```diff url = "https://ftp.mozilla.org/pub/firefox/releases/127.0b6/linux-x86_64/en-US/firefox-127.0b6.tar.bz2", sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", +# Add a retry mechanism for downloading the archive ``` Suggestion importance[1-10]: 6Why: Adding a retry mechanism for downloads can significantly improve the robustness of the build process, especially in cases of network instability. | 6 |
Security |
Add a comment to verify the integrity of downloaded files using the provided sha256 hashes___ **Consider adding a comment or a mechanism to verify the integrity of the downloaded filesusing the provided sha256 hashes. This can help ensure that the files have not been tampered with.** [common/repositories.bzl [53-54]](https://github.com/SeleniumHQ/selenium/pull/14037/files#diff-25d82cd18102fed27d3202000e1f1b3a56a85ad2848236d91989cd30a3952401R53-R54) ```diff url = "https://ftp.mozilla.org/pub/firefox/releases/127.0b6/linux-x86_64/en-US/firefox-127.0b6.tar.bz2", -sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", +sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", # Ensure the file integrity using this sha256 hash ``` Suggestion importance[1-10]: 3Why: The suggestion to add a comment for verifying file integrity using sha256 is a good practice, but it's minor in impact as the hash itself inherently serves this purpose in the build tool. | 3 |
Best practice |
Verify that the URLs and SHA256 hashes are correct and correspond to the intended files___ **Ensure that the URLs and SHA256 hashes are correct and correspond to the intended files.This can be done by manually verifying the URLs and computing the SHA256 hashes of the downloaded files.** [common/repositories.bzl [53-54]](https://github.com/SeleniumHQ/selenium/pull/14037/files#diff-25d82cd18102fed27d3202000e1f1b3a56a85ad2848236d91989cd30a3952401R53-R54) ```diff url = "https://ftp.mozilla.org/pub/firefox/releases/127.0b6/linux-x86_64/en-US/firefox-127.0b6.tar.bz2", -sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", +sha256 = "368de7ef7109f943cfc2a18c458e07797b87fc6b306222f0dc3ba85c9dc757d3", # Verify this hash manually ``` Suggestion importance[1-10]: 3Why: While ensuring the correctness of URLs and hashes is important, the suggestion to manually verify them does not reflect a practical or scalable approach in code review or automated processes. | 3 |
**Action:** Ruby / Local Tests (safari, macos) / Local Tests (safari, macos) |
**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9239312313/job/25418465596) [❌] |
**Failed test name:** test_integration_selenium_webdriver |
**Failure summary:**
The action failed because multiple tests encountered a Selenium::WebDriver::Error::WebDriverError during the before(:suite) hook. The specific reasons for the failure include:/var/root/.local/share/gem/ruby/3.0.0/specifications .before(:suite) hook raised a Selenium::WebDriver::Error::WebDriverError due to an inability to connect. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: macOS ... 666: [32mINFO: [0mFrom Compiling src/google/protobuf/generated_message_tctable_lite.cc [for tool]: 667: [1mexternal/protobuf~/src/google/protobuf/generated_message_tctable_lite.cc:347:14: [0m[0;1;35mwarning: [0m[1munused function 'Offset' [-Wunused-function][0m 668: inline void* Offset(void* base, uint32_t offset) { 669: [0;1;32m ^ 670: [0m1 warning generated. 671: [32mINFO: [0mFrom Linking external/protobuf~/libprotobuf_lite.a [for tool]: 672: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: file: bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protobuf_lite/io_win32.o has no symbols 673: [32mINFO: [0mFrom Linking external/protobuf~/libprotobuf.a [for tool]: 674: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: file: bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protobuf/error_listener.o has no symbols ... 796: * `Zip::Entry` 797: * `Zip::InputStream` 798: * `Zip::OutputStream` 799: Please ensure that your Gemfiles and .gemspecs are suitably restrictive 800: to avoid an unexpected breakage when 3.0 is released (e.g. ~> 2.3.0). 801: See https://github.com/rubyzip/rubyzip for details. The Changelog also 802: lists other enhancements and bugfixes that have been implemented since 803: version 2.3.0. 804: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 0s disk-cache 805: [32m[1,863 / 1,864][0m [Sched] Testing //rb/spec/integration/selenium/webdriver:error-safari 806: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 1s local, disk-cache 807: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 9s local, disk-cache ... 817: [32m[1,865 / 1,866][0m 2 / 24 tests;[0m Testing //rb/spec/integration/selenium/webdriver:listener-safari; 24s local, disk-cache 818: [35mFLAKY: [0m//rb/spec/integration/selenium/webdriver:listener-safari (Summary) 819: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/listener-safari/test_attempts/attempt_1.log 820: ==================== Test output for //rb/spec/integration/selenium/webdriver:listener-safari: 821: [32mINFO: [0mFrom Testing //rb/spec/integration/selenium/webdriver:listener-safari: 822: |
This is an automated pull request to update pinned browsers and drivers
Merge after verify the new browser versions properly passing the tests and no bugs need to be filed