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

tests: separate e2e tests into a standalone module #1273

Closed
wants to merge 14 commits into from

Conversation

MSalopek
Copy link
Contributor

@MSalopek MSalopek commented Sep 7, 2023

Description

Closes: #1132

e2e tests were moved outside /tests directory and made independent of the rest of the repository.

The e2e tests can still be used normally, no workflows were broken and there's no impact on the Makefile commands.

Some small refactors were made along the way.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct type prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR does not change production code

@MSalopek MSalopek requested a review from a team as a code owner September 7, 2023 12:41
@MSalopek MSalopek self-assigned this Sep 7, 2023
@MSalopek MSalopek added the scope: testing Code review, testing, making sure the code is following the specification. label Sep 7, 2023
@github-actions github-actions bot added C:Testing Assigned automatically by the PR labeler C:Build Assigned automatically by the PR labeler labels Sep 7, 2023
@MSalopek MSalopek removed the C:Build Assigned automatically by the PR labeler label Sep 7, 2023
@github-actions github-actions bot added the C:Build Assigned automatically by the PR labeler label Sep 7, 2023
@@ -28,6 +28,8 @@ brew install jq

**Installing and running binaries**

This repository consists of multiple go modules. In order to have a smooth development experience, please add a `go.work` file. It is enough to copy and rename the `go.work.example` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

The go.work file is also in this PR, this makes me think it should not. Should we remove the go.work file itself from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the go.work file should not be commited.

It is commited for now because the testing pipelines fail if you don't commit a go work file. The go tool sees that there are 2 modules but does not know how to build and execute normal tests without instructions from go.work.

This can also be fixed by some make magic, e.g.:

# create a go.work file on the fly by renaming go.work.example
mv ./go.work.example ./go.work
go test (...)

I opted to not do that now since it would be more confusing than just commiting the file directly.

Notice how none of the make commands are actually changed, they just point to a different path (./e2e instead of ./testing/e2e).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense. Then maybe let's change that line in the README to account for the fact that the go.work file is in the repo already and keep it like this for now?

Copy link
Contributor

@p-offtermatt p-offtermatt left a comment

Choose a reason for hiding this comment

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

Nice work, did not think it would just work out of the box like this (well, as out-of-the-box as 44 files changed are). Just a small comment, but seems to work well.

Makefile Outdated

# run only happy path E2E tests using latest tagged gaia
test-gaia-e2e-short:
go run ./tests/e2e/... --tc happy-path --use-gaia
go run ./e2e/... --happy-path-only --use-gaia
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
go run ./e2e/... --happy-path-only --use-gaia
go run ./e2e/... --tc happy-path --use-gaia

Makefile Outdated

# run only happy path E2E tests using latest tagged gaia
# usage: GAIA_TAG=v9.0.0 make test-gaia-e2e-short-tagged
test-gaia-e2e-short-tagged:
go run ./tests/e2e/... --tc happy-path --use-gaia --gaia-tag $(GAIA_TAG)
go run ./e2e/... --happy-path-only --use-gaia --gaia-tag $(GAIA_TAG)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, what is the reason to go back to the 'old' flag and not using --tc
if I'm not mistaken that's not even supported anymore. did you test that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That test has not been working for a while. Gaia is on v45, the ICS main is on v47.
Since e2e was coupled to the ICS version, some of the commands to execute against a gaia node are not valid and cannot be executed. Notice that most of that was intended for gaia v9.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed, thanks!

@@ -0,0 +1,6 @@
go 1.20
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these in the gitignore?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm addressed in other comment

Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

LGTM

@MSalopek MSalopek marked this pull request as draft September 22, 2023 14:24
@MSalopek
Copy link
Contributor Author

Drafting until v50 is in to mitigate some risks

@MSalopek MSalopek added the other: decayed Stale issues that need follow up from commentators. Were closed for inactivity label Apr 26, 2024
@MSalopek
Copy link
Contributor Author

Closing as the PR has decayed and takes a lot of work to get it working again.
It can be used as an example at a later time if we wish to proceed with the work.

@MSalopek MSalopek closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Build Assigned automatically by the PR labeler C:Testing Assigned automatically by the PR labeler other: decayed Stale issues that need follow up from commentators. Were closed for inactivity scope: testing Code review, testing, making sure the code is following the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make e2e it's own module (de-couple from ICS lib and versioning scheme)
4 participants