Skip to content

acc: Add CloudSlow config setting#2470

Merged
denik merged 4 commits intomainfrom
denik/acc-CloudLong
Mar 11, 2025
Merged

acc: Add CloudSlow config setting#2470
denik merged 4 commits intomainfrom
denik/acc-CloudLong

Conversation

@denik
Copy link
Copy Markdown
Contributor

@denik denik commented Mar 11, 2025

Changes

New bool config setting CloudSlow in acceptance tests. If set, it enables this test on Cloud but skips it in -short setting there. It does not affect local runs, "Slow" is only applied to Cloud. The reason is that we won't need to wait for cluster with mocked testserver.

Additionally, this setting enables -tail if -v is already enabled.

Why

Certain tests that use "bundle run" are too long due to starting a cluster.

Tests

Using this option in #2471

@denik denik force-pushed the denik/acc-CloudLong branch from fde8b13 to 6e02c5f Compare March 11, 2025 14:16
@denik denik temporarily deployed to test-trigger-is March 11, 2025 14:16 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 11, 2025 14:18 — with GitHub Actions Inactive
@denik denik temporarily deployed to test-trigger-is March 11, 2025 14:19 — with GitHub Actions Inactive
@denik denik enabled auto-merge March 11, 2025 14:20

// If true, this test is not run if -short is passed and cloud env is configured
// This implies Cloud = true.
CloudLong *bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we use -short in integration runs from a PR? That is, is this flag meant to annotate tests that run only in our nighlies?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes.


// If true, this test is not run if -short is passed and cloud env is configured
// This implies Cloud = true.
CloudLong *bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about:

Suggested change
CloudLong *bool
Expensive *bool
  1. We already have a Cloud tag that can be composed with it.
  2. Normally Long refers to a number, expensive will be more explicit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could have a separate flag for controlling short behavior, but here I'm specifically adding short scoped to cloud runs only. The reason is that all tests should be fast locally with mock server, so if a test works both as a local test and cloud test, it would only be slow on cloud.

"Long" is used because it's the opposite of "Short", "Expensive" does not have this property.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Long is indeed confusing here, what about CloudSlow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CloudSlow is fine too, updated.


if testing.Verbose() {
// Combination of CloudLong and -v auto-enables -tail
tailOutput = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unrelated, but tailing the output can be the default behaviour for all tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's needed, since we already record it and show a diff, it's only useful to monitor and review timestamps of long running tests.

@denik denik requested a review from shreyas-goenka March 11, 2025 15:04
@denik denik temporarily deployed to test-trigger-is March 11, 2025 15:25 — with GitHub Actions Inactive
@denik denik changed the title acc: Add CloudLong config setting acc: Add CloudSlow config setting Mar 11, 2025
@denik denik disabled auto-merge March 11, 2025 15:48
@denik denik merged commit 992425e into main Mar 11, 2025
8 of 9 checks passed
@denik denik deleted the denik/acc-CloudLong branch March 11, 2025 15:48
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