Skip to content

Add xtask support for refresh slide list. #2774

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

Merged
merged 4 commits into from
Aug 1, 2025

Conversation

egithinji
Copy link
Member

Adds support for a refresh-slide-list argument when running the command cargo xtask web-tests. Allows one to also specify an optional book html directory if one uses the refresh-slide-list argument. Fixes #2744 .

@michael-kerscher
Copy link
Collaborator

I'm sorry for the huge delay. I already prepared my review but somehow did not click the submit button...

@egithinji
Copy link
Member Author

Hi @michael-kerscher . I noticed a difference in behavior when passing a directory using the full path (e.g. cargo xtask web-tests -d /usr/.../comprehensive-rust/book/html) vs from relative the workspace directory (e.g. cargo xtask web-tests -d book/html).

When using the full path, the slide list gets recreated but it seems like the actual testing of the slide dimensions doesn't happen. When using the shorter path from the workspace directory, it seems to work as expected. Is there a difference in how these paths are treated from within the web tests?

@michael-kerscher
Copy link
Collaborator

Hmm, if that is an issue for the actual test runner, this would be some interaction with npm or something inside wdio. I need to look at this

Copy link
Collaborator

@michael-kerscher michael-kerscher left a comment

Choose a reason for hiding this comment

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

Absolute paths are fine, relative paths have conflicting usage, once with the CARGO_WORKSPACE_DIR/ and once with CARGO_WORKSPACE_DIR/tests/ (due to different working directories being used). You can resolve this issue by just converting the relative path to an absolute path. This also checks the directory and can provide early errors if the directory is wrong or does not exist

Copy link
Collaborator

@michael-kerscher michael-kerscher 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 to me now!

@egithinji egithinji merged commit 570a726 into google:main Aug 1, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement parameter for cargo xtask web-tests to refresh slide list to test in the web tests
2 participants