Make test-manager CLI simpler

[Serock3]

Includes a few quality of life improvments, most importantly making previous app and GUI e2e test packages optional.

---

This change is 

[linear]

DES-1039 Make launching test-manager easier for individual tests

[faern]

Reviewed 6 of 7 files at r1.
Reviewable status: 4 of 11 files reviewed, 6 unresolved discussions (waiting on @Serock3)

---

test/test-manager/src/main.rs line 97 at r1 (raw file):

        /// The CLI interface must be compatible with the upgrade test.
        #[arg(long, short)]
        previous_app: Option<String>,

Can the docs be updated to reflect what happens if this is not specified? Given that it's part of the CLI interface docs, it makes sense to document it pretty well.

---

test/test-manager/src/package.rs line 38 at r1 (raw file):

        log::warn!("No previous app version specified");
        None
    };

If the logging is not considered important, this could be simplified to just previous_app.map(|app| find_app(app, false, package_type)

---

test/test-manager/src/package.rs line 51 at r1 (raw file):
I find "graphics" to be a suboptimal wording here. It's the electron app e2e tests that can't run, right? That they might contain graphics is just an unrelated property is it not? Any e2e test we write in our electron app will be unable to run whether or not they test something that is primarily visual or not.

How about just:

Could not find UI e2e test binary.

---

test/test-manager/src/vm/provision.rs line 124 at r1 (raw file):
The question I ask here is "Why is it not copied?!". Saying not copying X implies there is an X and it was decided to not copy it. But here it does not exist. How about something like:

No previous app to send to remote

---

test/test-manager/src/vm/provision.rs line 130 at r1 (raw file):

            .context("Failed to send ui_e2e_tests_path to remote")?;
    } else {
        log::warn!("Not copying GUI e2e test to remote")

Same as above.

---

test/test-manager/src/vm/provision.rs line 173 at r1 (raw file):

            .ui_e2e_tests_path
            .map(|path| path.file_name().unwrap().to_string_lossy().into_owned())
            .unwrap_or_default(),

Can we lift these out to variables instead of inlining them as arguments directly in format!? They are pretty long expressions and it's not trivial to see how they differ/what they mean. By assigning them to a variable with a nice name it becomes easier to see what args will end up containing/meaning.

[faern]

Reviewed 1 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 7 of 11 files reviewed, 9 unresolved discussions (waiting on @Serock3)

---

test/ci-runtests.sh line 201 at r5 (raw file):

        --account "${ACCOUNT_TOKEN:?Error: ACCOUNT_TOKEN not set}" \
        --current-app "${cur_filename}" \
        --old-app "${prev_filename}" \

Why is "old" better than "previous"? I'd argue that old is less specific and more vague. I guess the meaning of this in reality is "For tests that involve upgrading from one version of the app to current-app, this specifies the version of the app to upgrade from"?

---

test/test-manager/src/container.rs line 32 at r3 (raw file):

    let status = cmd.status().await.unwrap_or_else(|e| {
        panic!("failed to execute [{:?}]: {}", cmd, e);

Is this not equivalent to using .expect() with the same message?

---

test/test-manager/src/logging.rs line 33 at r4 (raw file):

            logger.filter_module("hyper", log::LevelFilter::Info);
            logger.filter_module("rustls", log::LevelFilter::Info);
            logger.filter_level(log::LevelFilter::Info);

This seems to change the logging level of the entire test-manager? Why is this desired? It seems unrelated to simplifying the CLI

---

test/test-manager/src/main.rs line 97 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can the docs be updated to reflect what happens if this is not specified? Given that it's part of the CLI interface docs, it makes sense to document it pretty well.

Sorry. I did review this per commit. So when I left this comment I did not see that you later added these docs.

[Serock3]

Reviewable status: 6 of 11 files reviewed, 8 unresolved discussions (waiting on @faern)

---

test/ci-runtests.sh line 201 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Why is "old" better than "previous"? I'd argue that old is less specific and more vague. I guess the meaning of this in reality is "For tests that involve upgrading from one version of the app to current-app, this specifies the version of the app to upgrade from"?

I don't prefer "old", but clap doesn't allow short forms for flags that start with the same letter. Alternatively we could rename --package-folder to something starting with another letter, but I couldn't come up with anything good.

---

test/test-manager/src/container.rs line 32 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is this not equivalent to using .expect() with the same message?

If you put a format! inside .expect() then clippy complains about use of 'expect' followed by a function call, since it's eagerly evaluated.

---

test/test-manager/src/logging.rs line 33 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This seems to change the logging level of the entire test-manager? Why is this desired? It seems unrelated to simplifying the CLI

You can still specify the log level using RUST_LOG, which is done by ci-runtests.sh so its output will be the same. I just found the logs too verbose for my taste when running manually. The debug level logs are not really relevant to me when I am working on new e2e tests, so I consider it a more sane default. In general I think INFO should be the default for logs.

---

test/test-manager/src/package.rs line 38 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

If the logging is not considered important, this could be simplified to just previous_app.map(|app| find_app(app, false, package_type)

I opted for the cautious approach and added a warning, just in case someone forgets to set the previous app version now that it's optional. It will eventually give a pretty clear error message anyway, so it may be superfluous. I'll keep it unless you strongly think logging is unimportant.

---

test/test-manager/src/package.rs line 51 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I find "graphics" to be a suboptimal wording here. It's the electron app e2e tests that can't run, right? That they might contain graphics is just an unrelated property is it not? Any e2e test we write in our electron app will be unable to run whether or not they test something that is primarily visual or not.

How about just:

Could not find UI e2e test binary.

Sure

---

test/test-manager/src/vm/provision.rs line 124 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

The question I ask here is "Why is it not copied?!". Saying not copying X implies there is an X and it was decided to not copy it. But here it does not exist. How about something like:

No previous app to send to remote

Agree

---

test/test-manager/src/vm/provision.rs line 130 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Same as above.

Done.

---

test/test-manager/src/vm/provision.rs line 173 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can we lift these out to variables instead of inlining them as arguments directly in format!? They are pretty long expressions and it's not trivial to see how they differ/what they mean. By assigning them to a variable with a nice name it becomes easier to see what args will end up containing/meaning.

Sure. I did that and refactored a bit.

[faern]

Reviewed 1 of 2 files at r6.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @Serock3)

---

test/ci-runtests.sh line 201 at r5 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I don't prefer "old", but clap doesn't allow short forms for flags that start with the same letter. Alternatively we could rename --package-folder to something starting with another letter, but I couldn't come up with anything good.

You can manually specify the letter used for the short form. Or just not let every argument have a short form. I much prefer doing one of those over arbitrarily renaming stuff just to have unique first letters. Always prefer descriptive good names over submitting to trivial technical limitations IMO :P

Otherwise, what happens when you have one flag per letter in the alphabet except one... Will whatever flag that comes next have to find a decent word on that single letter to be allowed to be added? :D

Except for a few well known commonly used abbreviations like -o --output, -i --input or -i --ignore-case, -r --recursive, -f --force I actually personally think short flags is a bit of an anti-pattern. It makes reading commands shorter, but way harder 🙃

run-tests -o 2024.1 -c 2024.5 -a 123423423 is not very descriptive! I would not have any idea what that did 😅

---

test/test-manager/src/logging.rs line 33 at r4 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

You can still specify the log level using RUST_LOG, which is done by ci-runtests.sh so its output will be the same. I just found the logs too verbose for my taste when running manually. The debug level logs are not really relevant to me when I am working on new e2e tests, so I consider it a more sane default. In general I think INFO should be the default for logs.

Ok! Yeah, makes sense IMO

---

test/test-manager/src/package.rs line 38 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I opted for the cautious approach and added a warning, just in case someone forgets to set the previous app version now that it's optional. It will eventually give a pretty clear error message anyway, so it may be superfluous. I'll keep it unless you strongly think logging is unimportant.

We can keep it if you think it's important. I just figured this is probably more suitably logged somewhere else earlier/further up? Let's keep it.

---

test/test-manager/src/vm/provision.rs line 173 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Sure. I did that and refactored a bit.

Nice!

[Serock3]

Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @faern)

---

test/ci-runtests.sh line 201 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

You can manually specify the letter used for the short form. Or just not let every argument have a short form. I much prefer doing one of those over arbitrarily renaming stuff just to have unique first letters. Always prefer descriptive good names over submitting to trivial technical limitations IMO :P

Otherwise, what happens when you have one flag per letter in the alphabet except one... Will whatever flag that comes next have to find a decent word on that single letter to be allowed to be added? :D

Except for a few well known commonly used abbreviations like -o --output, -i --input or -i --ignore-case, -r --recursive, -f --force I actually personally think short flags is a bit of an anti-pattern. It makes reading commands shorter, but way harder 🙃

run-tests -o 2024.1 -c 2024.5 -a 123423423 is not very descriptive! I would not have any idea what that did 😅

I agree with your point about short forms, I don't actually use them very often myself. It's just that my explicit intent with the PR was to make running test-manager manually easier, so it seemed in line with that goal to have a short form for the new flag. Maybe a better solution is to remove the short form of --previous-app since most likely you want to skip specifying it anyway.

Ideally, I would like to change the name of both --current-app and --previous-app. I don't think those names make sense anymore. previous kind of implies that it had to be the version that's one release older than current-app. Since you likely will skip previous-app the flags could be changed to --app-package and --app-package-to-upgrade-from or something, but that causes conflicts with --account 🙃

[faern]

Reviewed 2 of 4 files at r5, 1 of 2 files at r6.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion

[faern]

Reviewed 7 of 8 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved