[py] Use generated Bidi files instead of hand curated ones#17266
Open
AutomatedTester wants to merge 44 commits intotrunkfrom
Open
[py] Use generated Bidi files instead of hand curated ones#17266AutomatedTester wants to merge 44 commits intotrunkfrom
AutomatedTester wants to merge 44 commits intotrunkfrom
Conversation
…revert unrelated requirements changes
… the window to the size we want
selenium-ci
added
C-py
cgoldberg
approved these changes
Mar 29, 2026
Contributor
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Member
|
there are still some linting/formatting errors in the new generation code |
AutomatedTester
force-pushed
the
cddl2py_bazel
branch
from
d6d11a4 to
b49a8eb
Compare
cgoldberg
requested changes
Mar 31, 2026
Member
cgoldberg
left a comment
cgoldberg
left a comment
There was a problem hiding this comment.
I fixed python.rake previously, but you force pushed over my commit :/ I just added comments again.
We should add the generated files to .gitignore
Also, run the formatter so it passes the lint job in CI.
| @@ -0,0 +1,1824 @@ | |||
| #!/usr/bin/env python3.10 | |||
| FileUtils.cp(resolved_src, File.join(bidi_dest, entry)) | ||
| end | ||
| end | ||
|
|
||
| %w[common/devtools common/linux common/mac common/windows].each do |dir| |
|
|
||
| resolved_src = File.symlink?(src) ? File.realpath(src) : src | ||
| FileUtils.cp(resolved_src, File.join(bidi_dest, entry)) | ||
| end |
Member
There was a problem hiding this comment.
do FileUtils.rm_f() on each destination file before copying, or this will fail when you run it a 2nd time.
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
…onment (#17257) #17251 fix loading "isDisplayed.js" and other resources in OSGI environment In OSGi, each bundle has its own classloader that can only see its own resources. `Read.class.getResource(...)` uses the classloader of the bundle containing class `Read`. It would not find resource "isDisplayed.js" since it's located in another bundle. The right solution is to use a class from the bundle that owns the resource.
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
| data = browser_data, | ||
| tags = tags + browser_tags, | ||
| size = size, | ||
| ) |
There was a problem hiding this comment.
_test_wrapper_test is defined without an args attribute, but the macro passes args = .... This will fail at analysis time with an unknown attribute error. Fix by adding an explicit args attribute to _test_wrapper_test and (since a symlinked executable won’t automatically receive those args) generate a small wrapper launcher script (sh/bat as appropriate) that invokes the underlying test_binary with the provided args.
Suggested change
| ) | |
| ) | |
| def _test_wrapper_test( | |
| name, | |
| test_binary, | |
| args, | |
| data = [], | |
| tags = [], | |
| size = None): | |
| """Create a test target that launches `test_binary` with the given args. | |
| This macro generates a small shell wrapper script that invokes the compiled | |
| test binary with the provided `args` and forwards any additional command line | |
| arguments. The wrapper script is then used as the actual test executable. | |
| """ | |
| launcher_name = name + "_launcher" | |
| launcher_script = launcher_name + ".sh" | |
| native.genrule( | |
| name = launcher_name, | |
| srcs = [], | |
| outs = [launcher_script], | |
| tools = [test_binary], | |
| cmd = ( | |
| "echo '#!/usr/bin/env bash' > $@ && " | |
| "echo '\"$(location {test_binary})\" {args} \"$$@\"' >> $@ && " | |
| "chmod +x $@" | |
| ).format( | |
| test_binary = test_binary, | |
| args = " ".join(args), | |
| ), | |
| ) | |
| native.sh_test( | |
| name = name, | |
| srcs = [launcher_script], | |
| data = data + [test_binary], | |
| tags = tags, | |
| size = size, | |
| ) |
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:AllTests | ||
| bazel test //dotnet/test/common |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:ElementFindingTest | ||
| bazel test //dotnet/test/common:ElementFindingTests |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| ```shell | ||
| bazel test //dotnet/test/common:ElementFindingTest-edge | ||
| bazel test //dotnet/test/common:ElementFindingTests-edge |
There was a problem hiding this comment.
The updated CI and Bazel BUILD changes in this PR move the main .NET webdriver tests to //dotnet/test/webdriver, but these README commands still reference //dotnet/test/common. Update the examples to the new target(s) so the instructions remain runnable.
|
|
||
| location = allowed.get((platform_name, arch)) | ||
| if location is None: | ||
| raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}") |
There was a problem hiding this comment.
This error message uses sys.platform, but the lookup now normalizes to platform_name for BSD variants. When a combination is unsupported, the message can be misleading (e.g., showing freebsd13/... even if normalization occurred). Prefer reporting the normalized platform_name (and the normalized arch) used for the actual lookup.
Suggested change
| raise WebDriverException(f"Unsupported platform/architecture combination: {sys.platform}/{arch}") | |
| raise WebDriverException( | |
| f"Unsupported platform/architecture combination: {platform_name}/{arch}" | |
| ) |
Comment on lines
+88
to
+94
| cmd_parts = [ | ||
| "rm -rf '%s'" % working_dir, | ||
| "mkdir -p '%s'" % working_dir, | ||
| ] + copy_cmds + [ | ||
| "cp '{src}' '{dir}/project.nuspec'".format(src = nuspec.path, dir = working_dir), | ||
| "cp '{src}' '{dir}/project.csproj'".format(src = csproj_file.path, dir = working_dir), | ||
| "cd '%s'" % working_dir, |
There was a problem hiding this comment.
The refactor replaces the zipper-based layout with shell commands (rm, mkdir -p, cp). If this rule is expected to run on Windows hosts (or any environment without a POSIX shell/coreutils), it will break. Consider either (a) restoring a platform-neutral file layout step (zipper or a small cross-platform tool), or (b) using ctx.actions.copy/ctx.actions.write where possible and minimizing reliance on external shell utilities.
| [OneTimeSetUp] | ||
| public async Task RunBeforeAnyTestAsync() | ||
| { | ||
| Log.SetLevel(LogEventLevel.Trace); |
There was a problem hiding this comment.
Setting the global log level to Trace for all webdriver tests can significantly increase output volume and slow down CI/test runs (and may also make failures harder to triage due to noise). Consider gating this behind an environment variable or test run setting, defaulting to a less verbose level.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
💥 What does this PR do?
Deletes all the hand curated bidi code in the python tree and uses the generated code created in #16914 . Once that PR is approved we can merge this into there or do it into trunk as a 2nd PR. #16914 MUST BE FIRST INTO TRUNK
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes