Skip to content

acc: extract common repls; support order for repls #2972

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
Jun 2, 2025

Conversation

denik
Copy link
Contributor

@denik denik commented Jun 2, 2025

Changes

  • New timestamp and number related replacements in root test.toml
  • New attribute on [[Repls]] object: Order. This defines order in which replacements are applied.
  • Remove [TEST_JOB_ID+0] replacement, little value and does not work when making test support both local and cloud.

Why

Different tests are re-doing the same work with different approaches, this adds standard way to do this.

denik added 4 commits June 2, 2025 15:29
- Use common replacement for timestamps and numbers
- Add Order property to [[Repls]]. Lowest Order is applied first. Default is 0.
#[[Repls]]
#Old = '\d+\?o=\d+'
#New = '[NUMBER]'

Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

[[Repls]]
Old = '\?o=\[NUMID\]'
New = ''
Order = 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove both if they are stable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not stable, ?o= suffix is completely missing in some workspaces. Added this comment.

[[Repls]]
Old = '17\d{11}'
New = '"UNIX_TIME_MILLIS"'

Copy link
Contributor

Choose a reason for hiding this comment

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

This was quoted to keep the request log valid JSON.

I recall discussing this with @shreyas-goenka a while back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Why does it need to be valid json?

Copy link
Contributor

Choose a reason for hiding this comment

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

To extract relevant chunks with jq. IIRC this was the case for telemetry tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should work. Replacements are applied after script is finished running. During script run it's unprocessed.

@denik denik temporarily deployed to test-trigger-is June 2, 2025 14:22 — with GitHub Actions Inactive
@denik denik requested a review from pietern June 2, 2025 14:23
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

OK, then I don't recall what the issue was.

@denik denik temporarily deployed to test-trigger-is June 2, 2025 14:32 — with GitHub Actions Inactive
@denik denik enabled auto-merge June 2, 2025 14:39
@denik denik added this pull request to the merge queue Jun 2, 2025
Merged via the queue into main with commit 1dc39ff Jun 2, 2025
10 checks passed
@denik denik deleted the denik/acc-common-replacements branch June 2, 2025 14:57
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.

2 participants