Skip to content
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

Initial version of UI tests. #1115

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Nov 5, 2022

This is a very initial draft, but the idea is to add UI tests (which we'll add more as this gets further along).

UI tests still to add:

  • Remote
  • Remote with incorrect endpoint
  • Invalid image platform
  • Invalid pre-build script
  • Invalid pre-build command
  • Invalid pre-build dockerfile
  • ... Many more config options
  • A few other config flags

Any suggestions for additional UI tests to add, please suggest them below. This also needs to add ci/test-ui.sh into the CI. This will close #1037. I'm also waiting for #1103 to be closed so I can add it to the UI tests.

@Alexhuszagh Alexhuszagh added enhancement meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog) labels Nov 5, 2022
src/cli.rs Show resolved Hide resolved
xtask/src/ui.rs Outdated Show resolved Hide resolved
xtask/src/ui.rs Outdated Show resolved Hide resolved
xtask/src/ui.rs Outdated Show resolved Hide resolved
xtask/src/ui.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member

Emilgardis commented Nov 5, 2022

I think instead of having methods to check the output we can have .stderr and .stdout files, i.e what trycmd does.

this way we can also present a diff and ensure every aspect is captured.

But this is definitely an interesting approach and may work very well 👍🏼

@Alexhuszagh
Copy link
Contributor Author

I've added the initial versions of trycmd (and discovered some very minor bugs in the process). There's a few issues, but it's mostly working well. Due to the limitations of the markdown format, we're using the toml/stderr/stdout for everything, so to avoid duplicating everything numerous times, there's a lot of symlinks.

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Nov 20, 2022

I've completed reworked how CLI parsing works, and added an extensive set of integration tests. By default, these are ignored because they're very slow to run. I've also added in a regression test for #1141. I've ensured this only runs once per target.

The ignored unittests are also run in the main test section, so we don't run them except when needed (since they are very slow).

xtask/src/main.rs Outdated Show resolved Hide resolved
xtask/src/temp.rs Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
Comment on lines +66 to +67
trycmd = "0.14.4"
snapbox = "0.4"
Copy link
Member

Choose a reason for hiding this comment

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

We could have this as its own workspace member alongside cross and xtask, not needed but just a thought

Copy link
Contributor Author

@Alexhuszagh Alexhuszagh Nov 21, 2022

Choose a reason for hiding this comment

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

Is there any major advantage to that over faster compile times for the test suite? Not saying that isn't a good goal, just wondering if there's something additional as well.

src/cli.rs Outdated Show resolved Hide resolved
src/tests.rs Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Nov 21, 2022

This needs to handle escape sequences as well:
https://github.com/cross-rs/cross/actions/runs/3509739225/jobs/5879045993#step:6:238

I've used --color never for some of them, but that's not enough.

ci_dir=$(realpath "${ci_dir}")
. "${ci_dir}"/shared.sh

main() {
Copy link
Contributor Author

@Alexhuszagh Alexhuszagh Nov 21, 2022

Choose a reason for hiding this comment

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

This needs a clippy -- --deny warnings just to ensure that -Zbuild-std is getting correctly added as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement meta issues/PRs related to the maintenance of the crate. no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UI test
2 participants