Skip to content

chore: skip timing-sensistive AgentMetadata test in the standard suite #7237

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

Merged
merged 7 commits into from
May 2, 2023

Conversation

ammario
Copy link
Member

@ammario ammario commented Apr 20, 2023

No description provided.

@ammario ammario self-assigned this Apr 20, 2023
@ammario ammario requested a review from mafredri April 20, 2023 23:42
@ammario ammario enabled auto-merge (squash) April 20, 2023 23:42
t.Parallel()
script := "echo -n hello"
if runtime.GOOS == "windows" {
script = "powershell " + script
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be working, CI failing with missing hello. Does it need quoting?

//
// Eventually, we should run all timing tests in a self-hosted runner.

var timingFlag = flag.Bool("timing", false, "run timing-sensitive tests")
Copy link
Member

@mafredri mafredri Apr 21, 2023

Choose a reason for hiding this comment

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

I think we won't be able to do go test ./... -timing unless all test packages import testutil, a shortcoming of using flags in tests. I think the only way around it would be to use build tags instead but maybe this won't be an issue.

Copy link
Member Author

@ammario ammario Apr 21, 2023

Choose a reason for hiding this comment

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

Hmm, I explored build tags but decided against it because it could lead to file bloat.

Let me know what you think about ba6f151.

Copy link
Member

@mafredri mafredri Apr 22, 2023

Choose a reason for hiding this comment

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

Hmm, I explored build tags but decided against it because it could lead to file bloat.

I was thinking it could be contained to the testutil package using the checking function you already implemented, so only 1 more file should be necessary. The build tag would then decide if the test becomes enabled or not.

Example:

//go:build timing

package testutil

var _ = func() any {
	timing = true
	return nil
}()

(Or use init, but the above avoids using that reserved name, tags/duped func in both files is another option.)

This would allow running the entire test suite with go test ./... -tags=timing. Or limiting to timing tests via go test ./... -tags=timing -run='_Timing$'.

Let me know what you think about ba6f151.

I think that looks alright since it requires an intent to run, otherwise generating go_tests.json would be too slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea, let's go that way. See new code.

@ammario ammario force-pushed the agent-metadata-timings branch from 0e1d093 to 38340aa Compare April 21, 2023 19:20
@ammario ammario disabled auto-merge April 21, 2023 19:58
@ammario ammario force-pushed the agent-metadata-timings branch 22 times, most recently from b58e61a to fb0ca0c Compare April 21, 2023 23:36
@ammario ammario force-pushed the agent-metadata-timings branch 6 times, most recently from 162226e to cfc1564 Compare April 21, 2023 23:45
@ammario ammario force-pushed the agent-metadata-timings branch 2 times, most recently from 26a1653 to b001437 Compare April 22, 2023 21:42
@ammario ammario force-pushed the agent-metadata-timings branch from 0e29349 to c95ff85 Compare April 22, 2023 22:24
@ammario ammario requested a review from mafredri April 22, 2023 22:24
@ammario ammario force-pushed the agent-metadata-timings branch from c95ff85 to ca21e39 Compare April 22, 2023 22:29
@ammario ammario force-pushed the agent-metadata-timings branch from ca21e39 to 05127e6 Compare April 22, 2023 22:32
for start := time.Now(); time.Since(start) < testutil.WaitMedium; time.Sleep(testutil.IntervalMedium) {
md := client.getMetadata()
if len(md) != 2 {
panic("unexpected number of metadata entries")
Copy link
Member

Choose a reason for hiding this comment

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

Let's replace this with a require btw.

@ammario ammario enabled auto-merge (squash) April 24, 2023 09:20
@github-actions
Copy link

github-actions bot commented May 2, 2023

This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity.

@github-actions github-actions bot added the stale This issue is like stale bread. label May 2, 2023
@ammario ammario merged commit 465fe86 into main May 2, 2023
@ammario ammario deleted the agent-metadata-timings branch May 2, 2023 10:41
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stale This issue is like stale bread.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants