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.

Sign up for GitHub

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

Jump to bottom

feat: add variable wait when destroying offers #565

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add variable wait when destroying offers #565

wants to merge 1 commit into from

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Sep 5, 2024

Description

As a follow-up to #560, this PR aims to speed up tests when destroying offers. Instead of changing the hardcoded value of 5 minutes, we opt to use a variable instead.

This variable can be changed from the test init to speed up destroying offers. It is gated behind an environment variable so that tests can still be run with the same behaviour as production if desired.

Type of change

  • Change in tests (one or several tests have been changed)

hmlanigan
hmlanigan reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

It looks like a bug which needs to be fixed is being glossed over.

@kian99
Copy link
Contributor Author

kian99 commented Sep 9, 2024

It looks like a bug which needs to be fixed is being glossed over.

I agree it looks like an underlying bug. The bug may be coming from Juju or the Terraform provider so it will take some effort to solve. In the interim I think this PR has value in making tests faster. Wdyt?

@hmlanigan hmlanigan added the state/codefreeze this pr cannot land until the code freeze has lifted. label Sep 10, 2024
@hmlanigan
Copy link
Member

I agree it looks like an underlying bug. The bug may be coming from Juju or the Terraform provider so it will take some effort to solve. In the interim I think this PR has value in making tests faster. Wdyt?

It will take some effort solve. If we've glossed over in the tests, it's easier to ignore and delay fixing. It reduces our motivation to fix it.

@kian99
Copy link
Contributor Author

kian99 commented Sep 12, 2024

It will take some effort solve. If we've glossed over in the tests, it's easier to ignore and delay fixing. It reduces our motivation to fix it.

I have to disagree here. We gain a lot by reducing the test time, iterating becomes faster, and PRs become easier to land.
My worry is that if we use the pain of slow tests to motivate us to solve the bug then we run the risk of continuing to sit with slow tests and an unsolved bug. What I'd prefer to see is prioritising the issue to solve it.

@hmlanigan hmlanigan removed the state/codefreeze this pr cannot land until the code freeze has lifted. label Sep 16, 2024
@kian99
feat: add variable wait when destroying offers
f68bb5b 
- Add a variable that can be changed from tests to speed up destroying offers.
@hmlanigan
Copy link
Member

I'd love to prioritize it, how the juju team has very little capacity to actually do the work right now. It is a significant amount of work to do.

To make PRs easier to land, we could also stop testing with 3 versions of terraform, and only test with the latest version. This is easier to do as the terraform client is a snap that is auto updated for folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmlanigan hmlanigan Awaiting requested review from hmlanigan

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kian99 @hmlanigan