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

Pulse removal not mentioned in release notes #86

Open
philippgille opened this issue Dec 30, 2022 · 5 comments
Open

Pulse removal not mentioned in release notes #86

philippgille opened this issue Dec 30, 2022 · 5 comments

Comments

@philippgille
Copy link

Hello 👋 , thank you very much for creating and maintaining this open source project!

I recently updated to a newer version of tparse (now v0.11.1) and some commands I ran in the past don't work anymore. Specifically it seems the flag -pulse is not supported anymore. But I don't see this mentioned in the release notes.

As the project is a v0, breaking changes are perfectly fine, so my suggestion is just to add a note in the release notes here on GitHub where -pulse was removed.


Additional info:

@mfridman
Copy link
Owner

Hey there, glad you found the project useful. And I'll try to include a better changelog for breaking changes!

Indeed, I removed the -pulse flag as I didn't have the bandwidth to think through a nice UX after the large refactor.

Question, would you be using this in CI or locally when running tests?

@mfridman
Copy link
Owner

mfridman commented Feb 7, 2023

Note to self, I was debugging another issue and noticed it's quite annoying not to see any output. So, a reminder to actually do this.

From https://github.com/cybertec-postgresql/pg_timetable

go1.20 test ./... -json -v -count=1 | tparse -all -smallscreen

┌───────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │             PACKAGE             │ COVER │ PASS │ FAIL │ SKIP  │
│─────────┼─────────┼─────────────────────────────────┼───────┼──────┼──────┼───────│
│  PASS   │  0.19s  │ pg_timetable/internal/api       │  --   │  2   │  0   │  0    │
│  PASS   │  0.12s  │ pg_timetable/internal/config    │  --   │  6   │  0   │  0    │
│  PASS   │  0.26s  │ pg_timetable/internal/log       │  --   │  15  │  0   │  0    │
│  FAIL   │ 270.17s │ pg_timetable/internal/migrator  │  --   │  6   │  3   │  0    │
│  FAIL   │ 322.12s │ pg_timetable/internal/pgengine  │  --   │  37  │  10  │  0    │
│  FAIL   │ 22.34s  │ pg_timetable/internal/scheduler │  --   │  14  │  1   │  0    │
│  PASS   │  0.16s  │ pg_timetable/internal/tasks     │  --   │  2   │  0   │  0    │
└───────────────────────────────────────────────────────────────────────────────────┘

The tests ran for ~5 min, and it would have been nice to see some output, maybe with an incrementing counter of tests per package or something..

@ti-mo
Copy link

ti-mo commented May 26, 2023

I'd like to get this behaviour back in some form. To me, it's rather surprising that no lines are emitted until go test has returned. Travis CI kills jobs that don't generate any output for 10 minutes, so currently the only option is to run with -follow, but our tests are really verbose on stdout. (I suggested splitting tparse and go test output between stdout and stderr in #77)

It would be nice if tparse's default behaviour would emit a PASS for a package as soon as it sees a line with Action, Package, Elapsed, but without Test., e.g. "Action":"pass","Package":"github.com/cilium/cilium/pkg/bpf","Elapsed":2.14, as that means go test finished testing a package.

This is similar to the default behaviour of 'go test', it emits ok github.com/cilium/cilium/pkg/bpf 2.134s lines as soon as a package is done testing.

@mfridman
Copy link
Owner

Yep, that's a good point. As currently implemented, it's either all (-follow) or nothing until go test is done.

The former can get very verbose, which is partially what this tool aims to solve, and the latter isn't great because there's no feedback to either the user (local development) or CI.

Iirc the default output for go test is stdout, is it odd that tparse would redirect the output to stderr and use stdout for its own purpose? Probably not, but figured it's worth raising.

@mfridman
Copy link
Owner

mfridman commented May 29, 2023

Alright, I took a stab at something slightly simpler (#95). I believe this will get you 90% of the way toward satisfying the CI use case. Although there is still an edge case where the last package could take longer than the CI timeout since the last output.

For these cases, I think a solution may be to add a -periodic flag which can be set to an arbitrary duration 1m and it'll guarantee to output something to inform CI that tests are still running. Maybe this could default to 1m when -progress is true. This is similar to what hashicorp/terraform#6163 does.


This will print a package-level summary as the tests are running. This will include FAIL, PASS, SKIP and no tests to run.

Example:

CleanShot 2023-05-28 at 22 02 30@2x

If this is acceptable, some nice-to-have would be to support color.

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

No branches or pull requests

3 participants