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

Change the test to polling instead of just a single sleep [main] #3112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joaopapereira
Copy link
Contributor

Description of the Change

  • Avoid flakyness due to time it takes to bind a service

@joaopapereira joaopapereira force-pushed the flakyness-on-service-command-tests branch from 9ebb964 to 586bd85 Compare August 15, 2024 17:43
Copy link
Member

@a-b a-b left a comment

Choose a reason for hiding this comment

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

Love this idea, let's see if this helps.

@weresch
Copy link
Contributor

weresch commented Aug 15, 2024

issue (blocking): I tried running this and found on occasion it failed, I think it's due to the order the apps can appear in the output. Sometimes APP2 comes first:

  OUT: Showing bound apps:
  OUT:    name                                        binding name                           status             message
  OUT:    APP2-1a263f4e-a731-4247-7fea-9dea533cdef0   ca204ee2-67cd-455e-68cf-afe0a557e805   create succeeded
  OUT:    APP1-cdd7753f-8c75-45e8-5cf7-37b5d063e594   e1c2f039-3950-48cf-613b-a09534ddf8d4   create succeeded

The test may need to be changed to validate that both appear but not rely on the order of them.

integration/v7/isolated/service_command_test.go Outdated Show resolved Hide resolved
integration/v7/isolated/service_command_test.go Outdated Show resolved Hide resolved
integration/v7/isolated/service_command_test.go Outdated Show resolved Hide resolved
integration/v7/isolated/service_command_test.go Outdated Show resolved Hide resolved
@joaopapereira joaopapereira force-pushed the flakyness-on-service-command-tests branch from 586bd85 to 69acaba Compare August 16, 2024 17:53
Comment on lines +180 to +183
ContainSubstring(`%s\s+%s\s+create succeeded\s*\n`, appName1, bindingName1),
ContainSubstring(`%s\s+%s\s+create succeeded\s*\n`, appName2, bindingName2),
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ContainSubstring(`%s\s+%s\s+create succeeded\s*\n`, appName1, bindingName1),
ContainSubstring(`%s\s+%s\s+create succeeded\s*\n`, appName2, bindingName2),
))
))
sessionContents := string(session.Out.Contents())
Expect(strings.Contains(sessionContents, fmt.Sprintf(`%s %s create succeeded`, appName1, bindingName1))).To(BeTrue())
Expect(strings.Contains(sessionContents, fmt.Sprintf(`%s %s create succeeded`, appName2, bindingName2))).To(BeTrue())

Copy link
Contributor

Choose a reason for hiding this comment

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

I found the test can fail if app2 is listed first. If we get the session contents and search it for substrings the order doesn't matter.

@joaopapereira joaopapereira force-pushed the flakyness-on-service-command-tests branch 5 times, most recently from 589b428 to 16ec389 Compare August 22, 2024 14:32
@joaopapereira joaopapereira force-pushed the flakyness-on-service-command-tests branch 2 times, most recently from 2e4ad12 to 0e35f04 Compare August 27, 2024 15:22
@joaopapereira joaopapereira force-pushed the flakyness-on-service-command-tests branch from 0e35f04 to 1f75b01 Compare September 9, 2024 16:25
- Avoid flakyness due to time it takes to bind a service

Signed-off-by: João Pereira <[email protected]>
@a-b a-b force-pushed the flakyness-on-service-command-tests branch from 1f75b01 to c474fbb Compare October 4, 2024 13:34
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