Fix nightly

[erikbosch]

Testing how we can get rid of the problem with -Z errors.

[SebastianSchildt]

Is it a really a good idea to REQUIRE nightly toolchains to buold databroker (if I understand correctly)

If so, what is our really, really good reason for it?

[erikbosch]

If I understand things correctly.

• This installs the nightly toolchain, but the nightly toolchain is only used if requested
• We need the nightly toolchain for our test, at least as long as we want to use --report-time which is an unstable option and thus (most of the time?) only available in nightly tool-chain. I actually do not know why this test has worked now and then in the past. If "default" sometimes have been nightly anyway? I actually do not know what controls what cargo version we use for building, i.e. how that is controlled
• The nightly will only be used when we use +nightly

I guess we alternatively could refactor the test case to not rely on the unstable feature

[erikbosch]

Did some tests removing --report-time and the json report format (which also is experimental). The problem then is that it seems difficult to capture the test result. On failure exit code is still 0, and it is not as easy as searching for "failure" or "error" in stdout it seems. So in short, verifying test results seems easier if using nightly. The main build of databroker anyway happen inside the docker container, so whatever we set in the *.yml file should not matter.

[argerus]

Yes, nightly is only used for running the tests (not building databroker), and as Erik mentions it's needed only if we want these output files.

Originally, this output was used to create a Github comment in the corresponding PR with a fancy test summary. This was removed because of lack of permissions when migrated to the kuksa repo.

[SebastianSchildt]

I see that this does not influence the "container builds", but doing this, are we not essentially also building databroker used for the tests with a nightly toolchain? I.e. what we test is build with a different toolchain than what we release (I do understand, this was not perfectly in sync before, except for the integration tests, but now it is nightly vs. stable).

If this is the only (simple) way, maybe we need to live with it for now. Just want to confirm whether my understanding is correct. (and why did this even break now?)

[erikbosch]

My theory so far:

• In https://github.com/eclipse/kuksa.val/blob/master/.github/workflows/kuksa_databroker_build.yml we have a lot of references to version: latest. This I suspect gets "latest master", which often is a "nightly" version, but sometimes a released version. Which could explain why we only sometimes gets an error for -Z

If you look at old "successful" builds like https://github.com/eclipse/kuksa.val/actions/runs/5120892422/jobs/9207950580 you can see that they anyway was not successful, there are errors like:

 error: the following required arguments were not provided:
  --test-threads <TEST_THREADS>

(but they did not cause the build to fail which is a bit strange IMHO)

As I said before, we could remove the dependency and instead try to do some greps on the output of the tests and that way try to figure out if there are any errors. Just let me know if you think that is the preferred approach.

[argerus]

cargo test does return non-successful return codes when a test fails. At least for the "normal" tests.

So, I think the issue is specific to the cucumber tests and it's runner.

#[tokio::main]
async fn main() {
    // databroker::init_logging();

    let opts = cli::Opts::<_, _, _, world::UnsupportedLibtestArgs>::parsed();

    DataBrokerWorld::cucumber()
        .with_writer(writer::Libtest::or_basic())
        .with_cli(opts)
        .after(|_feature, _rule, _scenario, _ev, world| {
            if let Some(w) = world {
                w.stop_databroker();
            }
            Box::pin(future::ready(()))
        })
        .run("tests/features/current_values.feature")
        .await;
}

So I think the preferred solution is to modify this runner to return non-success on test failures.

Separate from that, we might at some point want to report the results of our unit tests in some fancy way (again), which might require unstable features. But for simple success / failure, I don't think it's needed.

[SebastianSchildt]

Do we know how to achieve this in the cucumber runner? Maybe @sophokles73 knows or @rafaeling has looked into the cucumber magic already?

I agree with @erikbosch we should repair CI fast, but if there is a "similarly" easy way to achieve this without adding deps on "nightly" toolchain (that just "feels" bad to me) I would prefer that

In that case we could revisit "fancy" reporting later, but since we as of today are not using it anyway we wouldn't loose anything...

[argerus]

Replacing run with run_and_exit seems to do the trick..

diff --git a/kuksa_databroker/databroker/tests/current_values.rs b/kuksa_databroker/databroker/tests/current_values.rs
index a174f37..82183ce 100644
--- a/kuksa_databroker/databroker/tests/current_values.rs
+++ b/kuksa_databroker/databroker/tests/current_values.rs
@@ -180,6 +180,6 @@ async fn main() {
             }
             Box::pin(future::ready(()))
         })
-        .run("tests/features/current_values.feature")
+        .run_and_exit("tests/features/current_values.feature")
         .await;
 }

[erikbosch]

@argerus - don't we need clone here as well?

[argerus]

No, we don't use it (further) after this point.

(and if clone() was actually needed, it wouldn't compile)

[erikbosch]

@argerus - shall we better keep or remove it?

[argerus]

Since this is a test, I left it there as someone at some point found it useful.

I guess a "better" option would be to turn down the default log level, but I didn't put any effort into it.

[erikbosch]

@SebastianSchildt - could you check if the PR is acceptable from your side. If we want to refactor test cases to not use experimental features we could do it later, for me the most important short time solution is to make sure CI works.

[argerus]

If we want to refactor test cases to not use experimental features

I don't think the test cases use any experimental features. Only the test reporting.

[erikbosch]

@SebastianSchildt @argerus

Now the dependency to experimental features are gone.
I actually removed the whole "cargo test" section as the tests are included as well when you do the tarpaulin part to get coverage. I verified that the build fails if there is an error in the cucumber tests.

[erikbosch]

If we remove the "unstable part" we cannot do the result upload part. less important as I see it as the log anyway is visible in stdout.
But if we do not have the report part there is no need to have it as tests anyway are run by "cargo tarpaulin" below

[SebastianSchildt]

looking good to me