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

Fix: integration test 025 is not isolated #735

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

sagilaufer1992
Copy link
Contributor

@sagilaufer1992 sagilaufer1992 commented Oct 25, 2023

Issue & Steps to Reproduce / Feature Request

test 025_notifications uses all_notifications, and it fetches data from other tests, such as 017_notification, that may become unavailable during test 025's run.

Solution

make the test rely solely on its own data

QA done

ran the test locally to make sure it works on its own. deliberately failing it is much harder, didn't happen sporadically (that we caught) until a few days prior to the PR

depends_on = [env0_notification.test_notification_1, env0_notification.test_notification_2]
}

data "env0_notification" "test_notification_1" {
Copy link
Contributor

@avnerenv0 avnerenv0 Oct 25, 2023

Choose a reason for hiding this comment

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

why not just use

locals {
  test_notification_1_name = "notification123-${random_string.random.result}-1"
}

data "env0_notification" "my_notification" {
  name = locals.test_notification_1_name
}

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we wish to see that the all_notifications works as expected too, so I suggest we utilize its data, so if it got broken for some reason the test will fail

Copy link
Contributor

@chpl chpl Oct 25, 2023

Choose a reason for hiding this comment

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

agree with @avnerenv0 .
can be replaced with post condition on the all_notifications data

    postcondition {
      condition     = contains(self.names, local.test_notification_1_name)
      error_message = "One of the notifications not found"
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0c08f76

used the resource name, and used output for the all_notifications verification, that it got one of the notifications

@sagilaufer1992 sagilaufer1992 requested a review from chpl October 25, 2023 08:01
@github-actions github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Oct 25, 2023
@TomerHeber TomerHeber merged commit 6591632 into main Oct 25, 2023
4 checks passed
@TomerHeber TomerHeber deleted the fix-integration-test-25-uses-data-from-other-tests branch October 25, 2023 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix integration-tests ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants