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

feat: store test cluster logs and improve test readme [DET-3269] #657

Merged
merged 6 commits into from
Jun 11, 2020

Conversation

hamidzr
Copy link
Member

@hamidzr hamidzr commented Jun 5, 2020

Description

Test Plan

Commentary (optional)

@hamidzr hamidzr requested a review from hkang1 June 5, 2020 18:14
@cla-bot cla-bot bot added the cla-signed label Jun 5, 2020
Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

Thanks for adding to the README.md, they are super helpful! I ran into some small issues, and I attempted to diagnose wherever I could.

@@ -63,13 +63,15 @@ def teardown_cluster(config):
@contextmanager
def det_cluster(config):
try:
yield setup_cluster(config)
with open(str(results_dir.joinpath("cluster.stdout.logs")), "w") as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: The results_dir might not exist so we run into an error when it attempts to open the cluster.stdout.logs to write. A simple check of the directory and creating it if it doesn't exist should fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed up this section

webui/tests/bin/e2e-tests.py Show resolved Hide resolved
### Debugging Test Issues

For reproducing and catching test flakes there is a simple helper script `./bin/try-for-flake.sh`.
Just executing the script will run `make test` over and over until it hits a error.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: add that you can pass in a parameter to limit the number of times to run tests. eg) ./bin/try-for-flake.sh 30 to run make test 30 times.

Copy link
Member Author

@hamidzr hamidzr Jun 8, 2020

Choose a reason for hiding this comment

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

that would be helpful but the script doesn't support this option we can add that (separately) though.


To speed up this process you can:

1. try to avoid the whole cluster set up and tear down on each iteration
Copy link
Contributor

Choose a reason for hiding this comment

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

question: How is this done? Current setup process creates a new db, starts the cluster, sets up the users and experiments. The tests are strongly tied to the db setup. Wondering if there is something we can skip doing during each flake test?

Copy link
Member Author

@hamidzr hamidzr Jun 8, 2020

Choose a reason for hiding this comment

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

hmm it depends on if the target tests are re-runnable or not. I added more description here. let me know if that helps


1. try to avoid the whole cluster set up and tear down on each iteration
2. limit the test scope:
- use `.skip` on each unwanted test suite or `.only` on the target suite.
Copy link
Contributor

Choose a reason for hiding this comment

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

question: does this actually work with our cypress setup? It looks like it's a mix bag. I tried applying .only to the very first test in 02-loginout.spec.ts and it ran all the other test files fully, and when it came to 02-loginout.spec.ts it did run just the first test where I applied .only... Is that the correct behavior or is there a weird setup going on?

Copy link
Contributor

Choose a reason for hiding this comment

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

update: here's what worked for blocking full test files. If you apply x to the top level describe (xdescribe) it stops the entire describe block in that file from running. So to isolate the tests I want to run, I need to add x to all the other files.

Copy link
Member Author

Choose a reason for hiding this comment

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

it was mixed bag last time I used it. from my googling, it's about the Jest version that Cypress is using. It is supposed to work so I think it's safe to have it in the readme. I usually just delete the unwanted test suites.
cypress-io/cypress#5441
cypress-io/cypress#632
cypress-io/cypress#5345

@hkang1 hkang1 assigned hamidzr and unassigned hkang1 Jun 8, 2020
@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Jun 8, 2020
Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hkang1 hkang1 assigned hkang1 and hamidzr and unassigned hkang1 Jun 11, 2020
@hamidzr hamidzr changed the title feat: store cluster logs and improve test readme [DET-3269] feat: store test cluster logs and improve test readme [DET-3269] Jun 11, 2020
@hamidzr hamidzr merged commit aa48768 into determined-ai:master Jun 11, 2020
@hamidzr hamidzr deleted the e2e-tests-flake-catcher branch June 11, 2020 22:41
tayritenour pushed a commit to tayritenour/determined that referenced this pull request Apr 25, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit to eecsliu/determined that referenced this pull request Jun 23, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
stoksc pushed a commit that referenced this pull request Jun 26, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit that referenced this pull request Jun 28, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit that referenced this pull request Jun 28, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
stoksc pushed a commit that referenced this pull request Jul 20, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit that referenced this pull request Jul 24, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
stoksc pushed a commit that referenced this pull request Oct 17, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
azhou-determined pushed a commit that referenced this pull request Dec 7, 2023
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
wes-turner pushed a commit that referenced this pull request Feb 2, 2024
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
@dannysauer dannysauer added this to the 0.12.10 milestone Feb 6, 2024
rb-determined-ai pushed a commit that referenced this pull request Feb 29, 2024
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
amandavialva01 pushed a commit that referenced this pull request Mar 18, 2024
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit that referenced this pull request Apr 18, 2024
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
My prior fix to fixed Docker packaging, but the RPM
still had the wrong image name, so change the path in .goreleaser.yml
to the expected binary name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants