introduce flaky lines, remove unnecessary functionality #1

nikitaevg · 2024-02-04T09:59:32Z

We introduce flaky indicators that help distinguish flakes from real failures. We also remove all functionality that is not necessary for our project. And we also add test coverage for the action.

seanlip

Thanks @nikitaevg, sorry for the review delay! This looks good on the whole IMO :)

I just had a few comments -- haven't had time to make changes directly yet, but will try to make a PR later in the week if possible, so I figured I'd just leave the comments here for reference at least.

seanlip · 2024-02-13T10:46:22Z

README.md

-### `exit_error`
-
-The final error returned by the command
+**Optional** Specify which lines in output indicate that the failure is flaky. Note - if not specified, all failures are considered as real failures.


Do these have to be full lines or just substrings?

Just substrings, updated the description

seanlip · 2024-02-13T10:47:25Z

src/action.spec.ts

@@ -0,0 +1,128 @@
+import 'jest';


Add copyright notice.

seanlip · 2024-02-13T10:48:50Z

src/index.ts

-    // mimics native continue-on-error that is not supported in composite actions
-    process.exit(inputs.continue_on_error ? 0 : exitCode);
+    error(`Failed test with exception ${err.message}`);
+    process.exit(-1);


Shouldn't the exit code be 1?

Yep, thanks!

seanlip · 2024-02-13T10:56:00Z

src/action.spec.ts

+  });
+
+  it('detects real errors after flakes', async () => {
+    // The second file is used to indicate the flake.


I am getting a bit confused when reading these tests. In some cases the flaky string seems to be inside the temp files and in others it seems to be within the command / printed to the terminal. So I don't entirely understand what is going on here...

If the point of the second file is to print stuff to the terminal output then why not just echo it like we do with everything else?

In some cases the flaky string seems to be inside the temp files and in others it seems to be within the command / printed to the terminal.

It's always printed to the terminal, see this tests cats the file contents.

If the point of the second file is to print stuff to the terminal output then why not just echo it like we do with everything else?

We need different outputs in the first and second attempts - the first attempt is a flake, the second is not. Adjusted the comments a bit.

seanlip · 2024-02-13T10:56:35Z

src/action.spec.ts

+  expect(data).toBe(content);
+}
+
+describe('retry', () => {


Might be worth adding a test to distinguish matching by substring vs matching by line

Changed this test so it shows that we look for substrings

seanlip · 2024-02-13T11:00:35Z

src/action.ts

+
+    if (code === null) {
+      error('exit code cannot be null');
+      exitCode = -1;


Should this be 1?

Yeah, you're right

seanlip · 2024-02-13T11:02:08Z

src/action.ts

+
+function hasFlakyOutput(flaky_test_output_lines: string[], output: string[]): boolean {
+  const flakyIndicator = flaky_test_output_lines.find((flakyLine) =>
+    output.some((outputLine) => outputLine.includes(flakyLine))


It does seem like this is checking for substrings, maybe we should be calling the argument flaky_test_indicators or flaky_test_indicator_substrings instead (or something that doesn't have "lines" in the name).

Renamed to substrings_indicating_flaky_execution

seanlip · 2024-02-13T11:03:19Z

src/action.ts

+
+    if (!hasFlakyOutput(inputs.flakyTestOutputLines, output)) {
+      return exitCode;
+    }


Maybe add log after this to say it failed flakily and that we're restarting.

I don't mind, but note that we already will have
"Output contains flaky line: ${flakyIndicator}" log + "Starting attempt #${attempt}". IMO it's enough, but I can add if you want

Ah thanks, I missed that, sorry -- but I think the call to info() is in the wrong place. Can we move it to this function?

Based on its name, hasFlakyOutput() is something that would be expected to just return a boolean and shouldn't have any meaningful side effects, whereas we do want printing the log to be a deliberate action that we take in the execution.

I also suggest adding "Restarting test" or similar to that message since it connects the restart with the error.

Based on its name, hasFlakyOutput() is something that would be expected to just return a boolean and shouldn't have any meaningful side effects, whereas we do want printing the log to be a deliberate action that we take in the execution.

I agree with that, but I think it's very helpful to output which flake indicator was found, so we need to output something in the hasFlakyOutput method.

I moved logging to the calling side, and also left the log of what indicator was found.

nikitaevg

@seanlip PTAL

nikitaevg · 2024-02-16T19:06:58Z

README.md

-### `exit_error`
-
-The final error returned by the command
+**Optional** Specify which lines in output indicate that the failure is flaky. Note - if not specified, all failures are considered as real failures.


Just substrings, updated the description

nikitaevg · 2024-02-16T19:11:33Z

src/action.spec.ts

+  expect(data).toBe(content);
+}
+
+describe('retry', () => {


Changed this test so it shows that we look for substrings

nikitaevg · 2024-02-16T19:15:59Z

src/action.spec.ts

+  });
+
+  it('detects real errors after flakes', async () => {
+    // The second file is used to indicate the flake.


In some cases the flaky string seems to be inside the temp files and in others it seems to be within the command / printed to the terminal.

It's always printed to the terminal, see this tests cats the file contents.

If the point of the second file is to print stuff to the terminal output then why not just echo it like we do with everything else?

We need different outputs in the first and second attempts - the first attempt is a flake, the second is not. Adjusted the comments a bit.

nikitaevg · 2024-02-16T19:18:29Z

src/action.ts

+
+    if (code === null) {
+      error('exit code cannot be null');
+      exitCode = -1;


Yeah, you're right

nikitaevg · 2024-02-16T19:24:34Z

src/action.ts

+
+function hasFlakyOutput(flaky_test_output_lines: string[], output: string[]): boolean {
+  const flakyIndicator = flaky_test_output_lines.find((flakyLine) =>
+    output.some((outputLine) => outputLine.includes(flakyLine))


Renamed to substrings_indicating_flaky_execution

nikitaevg · 2024-02-16T19:26:17Z

src/action.ts

+
+    if (!hasFlakyOutput(inputs.flakyTestOutputLines, output)) {
+      return exitCode;
+    }


I don't mind, but note that we already will have
"Output contains flaky line: ${flakyIndicator}" log + "Starting attempt #${attempt}". IMO it's enough, but I can add if you want

nikitaevg · 2024-02-16T19:26:41Z

src/index.ts

-    // mimics native continue-on-error that is not supported in composite actions
-    process.exit(inputs.continue_on_error ? 0 : exitCode);
+    error(`Failed test with exception ${err.message}`);
+    process.exit(-1);


Yep, thanks!

nikitaevg · 2024-02-16T19:27:21Z

src/action.spec.ts

@@ -0,0 +1,128 @@
+import 'jest';


seanlip

Thanks @nikitaevg -- just one more thing and then I think it's good to go!

Also please update the first comment of this PR thread with a description of changes, thanks.

seanlip · 2024-02-17T17:04:42Z

src/action.ts

+
+    if (!hasFlakyOutput(inputs.flakyTestOutputLines, output)) {
+      return exitCode;
+    }


Ah thanks, I missed that, sorry -- but I think the call to info() is in the wrong place. Can we move it to this function?

Based on its name, hasFlakyOutput() is something that would be expected to just return a boolean and shouldn't have any meaningful side effects, whereas we do want printing the log to be a deliberate action that we take in the execution.

I also suggest adding "Restarting test" or similar to that message since it connects the restart with the error.

nikitaevg · 2024-02-17T18:07:50Z

@seanlip PTAL!

seanlip

Hi @nikitaevg -- this looks good, thanks! But I want to get the tests to run. Do you know how to add tests to the CI so that we can ensure this (and subsequent changes don't break anything)?

I'll try closing and reopening this PR to see if that works...

seanlip · 2024-02-17T18:22:22Z

Ok, hm. Could you go to .github/workflows/ci_cd.yml and clean up the unnecessary stuff? You can view runs here: https://github.com/oppia/retry/actions

nikitaevg · 2024-02-17T18:53:39Z

Ok, hm. Could you go to .github/workflows/ci_cd.yml and clean up the unnecessary stuff? You can view runs here: https://github.com/oppia/retry/actions

Ohhh, I didn't notice that file. It has so many tests, but I think the tests I provided are good enough.

So I want to leave ci_unit only and remove other jobs, would that be ok for you?

Also, maybe I'll do it in a separate PR? This PR becomes too big.

nikitaevg · 2024-02-17T18:54:42Z

@seanlip PTAL

seanlip · 2024-02-17T18:56:37Z

I suggest keeping ci_unit but dropping the codecov part. I defer to your judgment on integration -- but in general I suggest keeping tests that align with the remaining functionality (ok to write new ones in a separate PR).

Agree in general with not wanting PRs to be too big, but I think we do need some tests to run on presubmit before we can merge this. So I suggest fixing that in this PR since this is where you also modify the behaviour (I thought of doing them in a separate PR that gets merged before this one but I think that wouldn't work).

nikitaevg · 2024-02-17T19:03:08Z

@seanlip Let me remove all integration tests in this PR and add them in the following one, sounds good? I fixed the github actions, see the latest commit.

seanlip

Yup seems good. Thanks!

nikitaevg force-pushed the develop branch from f0d3d2c to c120c5f Compare February 4, 2024 13:45

feat: introduce flaky lines, remove unnecessary functionality

2938e3d

nikitaevg force-pushed the develop branch from c120c5f to 2938e3d Compare February 4, 2024 13:47

nikitaevg changed the title ~~test~~ introduce flaky lines, remove unnecessary functionality Feb 4, 2024

nikitaevg requested a review from seanlip February 4, 2024 13:58

nikitaevg assigned seanlip Feb 4, 2024

nikitaevg added 2 commits February 4, 2024 14:14

fix: readme

dc37a8c

feat: clarify readme

54f5eb0

nikitaevg mentioned this pull request Feb 7, 2024

add retries for flaky tests oppia/oppia#19665

Merged

5 tasks

seanlip requested changes Feb 13, 2024

View reviewed changes

seanlip assigned nikitaevg and unassigned seanlip Feb 13, 2024

nikitaevg added 2 commits February 16, 2024 19:33

fix: pr comments

b95e0ec

fix: return code

a12f329

nikitaevg commented Feb 16, 2024

View reviewed changes

nikitaevg assigned seanlip and unassigned nikitaevg Feb 16, 2024

nikitaevg requested a review from seanlip February 16, 2024 19:36

seanlip requested changes Feb 17, 2024

View reviewed changes

seanlip assigned nikitaevg and unassigned seanlip Feb 17, 2024

fix: pr comments

c7355c7

nikitaevg requested a review from seanlip February 17, 2024 18:07

nikitaevg assigned seanlip and unassigned nikitaevg Feb 17, 2024

seanlip reviewed Feb 17, 2024

View reviewed changes

seanlip closed this Feb 17, 2024

seanlip reopened this Feb 17, 2024

seanlip assigned nikitaevg and unassigned seanlip Feb 17, 2024

nikitaevg assigned seanlip and unassigned nikitaevg Feb 17, 2024

nikitaevg requested a review from seanlip February 17, 2024 18:54

seanlip assigned nikitaevg and unassigned seanlip Feb 17, 2024

fix: github actions

1b23770

nikitaevg assigned seanlip and unassigned nikitaevg Feb 17, 2024

seanlip approved these changes Feb 17, 2024

View reviewed changes

seanlip merged commit a9fb265 into master Feb 17, 2024
1 check passed

seanlip deleted the develop branch February 17, 2024 19:06

seanlip mentioned this pull request Feb 17, 2024

Add integration tests. #3

Closed

introduce flaky lines, remove unnecessary functionality #1

introduce flaky lines, remove unnecessary functionality #1

Conversation

nikitaevg commented Feb 4, 2024 • edited Loading

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitaevg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitaevg commented Feb 17, 2024

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Feb 17, 2024

nikitaevg commented Feb 17, 2024

nikitaevg commented Feb 17, 2024

seanlip commented Feb 17, 2024 • edited Loading

nikitaevg commented Feb 17, 2024

seanlip left a comment

Choose a reason for hiding this comment

nikitaevg commented Feb 4, 2024 •

edited

Loading

seanlip commented Feb 17, 2024 •

edited

Loading