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

Testing: reduce amount of logs produced, drop ounit2 #5684

Merged
merged 8 commits into from
Jun 17, 2024

Conversation

psafont
Copy link
Member

@psafont psafont commented Jun 12, 2024

Using alcotest allows us to ignore the output of the scale tests which produced more than 8 thousand lines, for no good reason.

Dune now generated http-lib and rrd-transport, which allows dropping the dependecy on ounit in 3 packages

As always, best reviewed commit-by-commit

@psafont
Copy link
Member Author

psafont commented Jun 12, 2024

There's a test failing in CI, but it's not being reported (!)

  Testing `Metrics scalability'.
  This run has ID `WMVA73ZI'.
  
  ..F
  
  ┌──────────────────────────────────────────────────────────────────────────────┐
  │ [FAIL]        V2          0   Write and read.                                │
  └──────────────────────────────────────────────────────────────────────────────┘
  [exception] Sys_error("/dev/shm/test-metrics3b5500.tmp: File exists")
              Re-raised at Stdlib__Filename.temp_file.try_name in file "filename.ml", line 348, characters 30-37
              Called from Dune__exe__Test_common.make_shared_file in file "ocaml/xcp-rrdd/test/transport/test_common.ml" (inlined), line 145, characters 2-63
              Called from Dune__exe__Test_scale.run_tests.(fun) in file "ocaml/xcp-rrdd/test/transport/test_scale.ml", line 86, characters 23-42
              Called from Stdlib__List.init_aux in file "list.ml", line 69, characters 12-15

This seems to be a (hopefully) very rare name clash. I've threaded a number so it's impossible to make the names clash when generating a large amount of temp files

@psafont psafont force-pushed the lesstestlogs branch 3 times, most recently from f84b153 to ce166d4 Compare June 12, 2024 15:41
@psafont psafont changed the title xapi-rrdd: change tests to reduce amount of logs produced Testing: reduce amount of logs produced, drop ounit2 Jun 13, 2024
let make_shared_file () =
Filename.temp_file ~temp_dir:"/dev/shm" "test-metrics" ".tmp"
let make_shared_file ?(k = 0) () =
Filename.temp_file ~temp_dir:"/dev/shm"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is causing test failures it might be because there are leftovers from a previous test, or we run multiple tests in parallel and they clash on the names. Filename.temp_file tries 1000 times to generate a name (using only 6 random hex digits though).

We could try to make this more unique by including the PID perhaps? And maybe register an at_exit handler to safe unlink them, so even if the test fails/gets interrupted we'd clean it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also increment a (global) number ourselves to make this more unique. I see we generate 4096 files in a test, so we could include PID + globally incrementing number in the suffix.

Copy link
Member Author

Choose a reason for hiding this comment

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

There could be a conflict between test_scale and test_unit when run simultaneously. Adding the number like I did would remove the possibility of conflict. We could add the name of the executable, or pid (as you say), or even function. But I don't see the point with the current solution

Using alcotest allows us to ignore the output of the scale tests which produced
more than 8 thousand lines, for no good reason.

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Use it to drop the dependency on ounit in the metadata

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Use it to drop the dependency on ounit in the metadata

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Generate wproxy's opam metadata with dune

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This also reduces amount of time from 27 to 19 seconds in my box

Now the opam metadata is generated by dune

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Also generate opam metadata with dune

This is the last user of ounit in the repository

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
The go sdk needs to tun tests, and opam needs these to be attached to a
package, so introduce xapi-sdk to do so

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This allows new packages to be introduces. Otherwise the CI fails when trying
to resolve the depexts of the newly-added package because they don't exist in
any opam repositories

Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
@edwintorok
Copy link
Contributor

Pinning packages in the CI is very very slow and requires a lot of disk space, and should be rarely needed, as you say only when new depexts get introduced, did you find a case where this caused a failure?
Would be good if opam supported determining depexts too without pinning.

In general we should probably reduce the number of packages we have if we want to pin (we can keep them as separate dune libraries still), but that would be the topic for a separate PR.

@psafont
Copy link
Member Author

psafont commented Jun 14, 2024

It's needed when adding a new package, because it's not present in the existin xs-opam.

When opam needs to install the depexts, it tries to resolve the dependencies of all the present packages. This fails because the package does not exist in xs-opam.

I agree that this is rare, and pinning makes the CI builds very slow. Using opam 2.1 in CI should make it somewhat faster, but it's still not great and it's nopt yet ready. Alternatively I can remove the pinning once the xapi-sdk package is added to xs-opam, and reenable pinning when / if we need to add a new package.

Another alternative can be keeping it disabled here, but add the package (without dependencies) in xs-opam, this allows the depext to work in xen-api. After the xen-api change is merged, xs-opam might start failing, but it can be fixed by updating the metadata from xen-api.

I think the latter is usually better, but currently all xs-opam builds are broken until the sdk package gets added, so we're in a egg-and-chicken situation, and the pinning unblocks us

@robhoes robhoes merged commit b944326 into xapi-project:master Jun 17, 2024
14 checks passed
@psafont psafont deleted the lesstestlogs branch June 18, 2024 09:23
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.

3 participants