Add xtask support for refresh slide list. #2774
Conversation
xtask/src/main.rs
Outdated
| WebTests, | ||
| WebTests { | ||
| /// (Re)generate the list of slides that are tested by the slide size test. | ||
| #[arg(short, long, group = "refresh")] |
There was a problem hiding this comment.
I thought about this for a bit. In the current build process, we use the environment variable that is set to specify the directory, so the refresh script does not require this as a commandline argument.
Reason for this was that initially I did not have a good way to provide the directory to the wdio environment so I used environment variables and reused this then for the script.
Now that we have a command that is calling both the refresh and the npm test in the same command, we could set the environment variable TEST_BOOK_DIR for the npm process directly in this command and always require the directory to be provided to the xtask subcommand
Then we could merge the --refresh_slide_list that is just a boolean with the --dir that requires the first one into one argument. The user needs to specify this anyways.
What do you think?
There was a problem hiding this comment.
Just to make sure I understand correctly, are you suggesting we do the following:
- do away with the --refresh_slide_list flag
- always refresh the slides when web-tests is called
- make the --dir arg mandatory
- assign the value passed via --dir to a
TEST_BOOK_DIRenv variable that is used by both create-slide.list.sh and npm?
There was a problem hiding this comment.
do away with the --refresh_slide_list flag
- yes, but rename --dir to something that is recognizable that this will refresh the slide list
always refresh the slides when web-tests is called
- no, refresh if --dir is provided
make the --dir arg mandatory
- no, if not provided, do not refresh
assign the value passed via --dir to aTEST_BOOK_DIR env variable that is used by both create-slide.list.sh and npm?
- I don't particularly care about how the create-slide-list.sh is called exactly, either is fine for the script, but as npm needs the TEST_BOOK_DIR anyways, I would like to see the environment variable set (and overwrite if already set, you don't need to check if it was set). So create-slide.list.sh does not need this explicitly set as an command line argument as it also reads that environment variable.
There was a problem hiding this comment.
Please check now. Left the arg named --dir but explained its purpose in the help. If --dir is set, create-slide-list.sh will run and the directory specified by --dir will also be set as the env variable for npm test.
egithinji
force-pushed
the
xtask-web-imprv
branch
from
54dd4f8 to
6c60578
Compare
| /// Tests all included Rust snippets. | ||
| RustTests, | ||
| /// Starts a web server with the course. | ||
| Serve, | ||
| Serve {}, |
There was a problem hiding this comment.
this empty struct could be just a plain enum variant (without {})
| /// Create a static version of the course in the `book/` directory. | ||
| Build, | ||
| Build {}, |
| Task::RustTests => run_rust_tests()?, | ||
| Task::Serve => start_web_server()?, | ||
| Task::Build => build()?, | ||
| Task::InstallTools {} => install_tools()?, |
There was a problem hiding this comment.
also remove the empty {} in this match
| println!("Running web tests..."); | ||
|
|
||
| let path_to_tests_dir = Path::new(env!("CARGO_WORKSPACE_DIR")).join("tests"); | ||
| let refresh_slides_script = "./src/slides/create-slide.list.sh".to_string(); |
There was a problem hiding this comment.
please use a path here
|
I'm sorry for the huge delay. I already prepared my review but somehow did not click the submit button... |
Adds support for a
refresh-slide-listargument when running the commandcargo xtask web-tests. Allows one to also specify an optional book html directory if one uses therefresh-slide-listargument. Fixes #2744 .