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

plan/example-rust: fix the test with new sdk and optimize build #1327

Merged
merged 12 commits into from Jun 14, 2022

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented May 2, 2022

  • update the SDK,
  • use a two-step "download package" - "build the main.rs" file in Dockerfile to speed-up iterations,
  • Replace the temporary sdk-rust rewrite by a release (Fix: unwrap event & fix groups typo. sdk-rust#14),
  • Update the test structure to have examples as part of integration,
  • Fix & start cleaning the test scripts

@laurentsenta laurentsenta changed the title plan/example-rust: update SDK and optimize build plan/example-rust: fix the test with new sdk and optimize build Jun 10, 2022
@laurentsenta
Copy link
Contributor Author

@mxinden @ackintosh ready for review!

@laurentsenta laurentsenta force-pushed the devx/improve-rust-tests branch 2 times, most recently from 76b4af8 to 0558859 Compare June 10, 2022 12:59
.circleci/config.yml Outdated Show resolved Hide resolved
@laurentsenta laurentsenta force-pushed the devx/improve-rust-tests branch 4 times, most recently from aca40f1 to ab728ee Compare June 10, 2022 13:43
@laurentsenta
Copy link
Contributor Author

Note:
This PR adds the rust example to CI tests,
so I'm also using it as an opportunity to split the different tests (exec, docker, k8s, examples) and do some cleaning.

plans/example-rust/Dockerfile Show resolved Hide resolved
plans/example-rust/Dockerfile Show resolved Hide resolved
println!("TODO: why is this necessary to make the test go green?");
let (client, _run_parameters) = testground::client::Client::new().await?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
println!("TODO: why is this necessary to make the test go green?");
let (client, _run_parameters) = testground::client::Client::new().await?;
let (client, _run_parameters) = testground::client::Client::new().await?;
client.wait_network_initialized().await?;

Would this resolve the failures as well @laurentsenta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks much more stable, thanks for the patch @mxinden

A quick note, the tests are still failing because the run.out file is empty, it's a file we collect (via --collect) at the end of the run. It looks like testground expects the SDK to write logs to a predefined folder,
Related code in the go-sdk:

https://github.com/testground/sdk-go/blob/d78f61eccaa37b0d941f98650bce5dbb8fd7b7f1/runtime/runenv_logger.go#L29

I'm going to skip this check:

Copy link
Member

Choose a reason for hiding this comment

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

The rust SDK doesn't implement this logger yet as far as I can tell,

The rust-sdk does have the stdout hack:

https://github.com/testground/sdk-rust/blob/00c280fd23b75f19f80a45bcfa752298f109640d/src/background.rs#L375-L388

It does not support run.out.

More importantly, it might be a good time to discuss logging, do we even need that parser, why not "just" rely on stdout and stderr? (and get rid of the pretty.go file at the same time).

Off the top of my head, that would be the cleaner solution.

@laurentsenta what do you think of simply adding a touch run.out && before the entrypoint in the Dockerfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a fair solution,

The way we test success with run.out made sort of sense without the --wait fix but it's arbitrary: https://github.com/testground/testground/blob/master/integration_tests/header.sh#L33-L38

If we both think it's worth looking into getting rid of run.* files, I'm in favor of not cluttering the images at all and starting relying on actual exit codes.

Copy link
Member

Choose a reason for hiding this comment

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

If we both think it's worth looking into getting rid of run.* files, I'm in favor of not cluttering the images at all and starting relying on actual exit codes.

Off the top of my head I don't have a use-case for it. That said, I am not a testground expert. 👍 for proposing to remove it in this repository.

@laurentsenta laurentsenta merged commit 5368947 into master Jun 14, 2022
@laurentsenta laurentsenta deleted the devx/improve-rust-tests branch June 14, 2022 11:35
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.

None yet

4 participants