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

TestOutputFlush always enabled #3146

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Jun 27, 2023

What?

It's better to have the tests always enabled since the initially mentioned 3s not affecting the total time of the k6's tests (we have worse cases).

E.g. see what it looks like on a local machine (with time), there is no significant difference between the runs as far as I can tell.

Without integration test:

go test -p 2 -race -timeout 800s -count 1 ./...  250.74s user 11.43s system 211% cpu 2:03.82 total
go test -p 2 -race -timeout 800s -count 1 ./...  230.61s user 9.15s system 197% cpu 2:01.62 total
go test -p 2 -race -timeout 800s -count 1 ./...  230.08s user 9.46s system 202% cpu 1:58.57 total
go test -p 2 -race -timeout 800s -count 1 ./...  209.84s user 8.65s system 199% cpu 1:49.31 total
go test -p 2 -race -timeout 800s -count 1 ./...  219.80s user 9.31s system 201% cpu 1:53.72 total

With integration test enabled:

go test -p 2 -race -timeout 800s -count 1 ./...  231.20s user 9.57s system 204% cpu 1:57.99 total
go test -p 2 -race -timeout 800s -count 1 ./...  237.81s user 10.10s system 204% cpu 2:01.31 total
go test -p 2 -race -timeout 800s -count 1 ./...  222.45s user 9.19s system 196% cpu 1:58.12 total
go test -p 2 -race -timeout 800s -count 1 ./...  213.54s user 9.04s system 199% cpu 1:51.39 total
go test -p 2 -race -timeout 800s -count 1 ./...  215.50s user 8.97s system 191% cpu 1:57.20 total

And in any case, 3s is almost nothing and acceptable as the price for the test,

Why?

Having this always enabled is better since, that way, anyone will run this.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#3117

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #3146 (3694632) into master (1df72a0) will increase coverage by 0.57%.
The diff coverage is n/a.

❗ Current head 3694632 differs from pull request most recent head b31eb76. Consider uploading reports for the commit b31eb76 to get more accurate results

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
+ Coverage   71.98%   72.56%   +0.57%     
==========================================
  Files         252      250       -2     
  Lines       19405    19400       -5     
==========================================
+ Hits        13969    14077     +108     
+ Misses       4542     4426     -116     
- Partials      894      897       +3     
Flag Coverage Δ
ubuntu 72.56% <ø> (+0.64%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 13 files with indirect coverage changes

@olegbespalov
Copy link
Contributor Author

🤔 or it's actually flaky https://github.com/grafana/k6/actions/runs/5390904305/jobs/9786958914

converting to draft

@olegbespalov olegbespalov marked this pull request as draft June 27, 2023 14:29
@codebien
Copy link
Contributor

codebien commented Jun 27, 2023

@olegbespalov Yes, they are flaky, it is a known issue. We have to sort the metrics slide by name.

@olegbespalov olegbespalov marked this pull request as ready for review June 28, 2023 06:13
@olegbespalov olegbespalov self-assigned this Jun 28, 2023
@olegbespalov olegbespalov merged commit b189496 into master Jun 29, 2023
@olegbespalov olegbespalov deleted the chore/cloud-output-v2-integration-test branch June 29, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants