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

Migrate tests to cucumber #78

Merged
merged 7 commits into from
Feb 22, 2024
Merged

Migrate tests to cucumber #78

merged 7 commits into from
Feb 22, 2024

Conversation

ZoopOTheGoop
Copy link
Contributor

As the title says, this will migrate the integration tests (or some subset) to Cucumber, currently under investigation.

Closes #65

@ZoopOTheGoop ZoopOTheGoop force-pushed the cucumber-tests branch 8 times, most recently from fa19df6 to 50475a0 Compare February 21, 2024 08:25
@ZoopOTheGoop ZoopOTheGoop requested a review from tim-hm February 21, 2024 08:28
@ZoopOTheGoop ZoopOTheGoop marked this pull request as ready for review February 21, 2024 08:28
@ZoopOTheGoop
Copy link
Contributor Author

ZoopOTheGoop commented Feb 21, 2024

This is more or less done. A few things should probably be renamed for consistency.

Unlike the old integration tests, the new cucumber tests really only test externally observable behavior (which I think is a strength). The one exception is double authentication. It's very hard to meaningfully tell the difference between double auths without just asking the DB if someone got added twice somehow, so that's the only bit of test SQL.

One side thing is because of the high volume of votes for chart testing, i ended up having to parallelize the vote generator. This is a little ugly, but I think it's for the best.

@ZoopOTheGoop ZoopOTheGoop force-pushed the cucumber-tests branch 5 times, most recently from dbeeba2 to bf282db Compare February 21, 2024 08:59
Copy link
Contributor

@tim-hm tim-hm left a comment

Choose a reason for hiding this comment

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

This is good work @ZoopOTheGoop. Just a few tweaks related to more how this would be used in practice please:

  • In your givens, you could do a quick check to see if infra is running correctly and bail with a message suggesting how the tests should be run (eg cargo make full-test or something else)
  • If you want to stick with cargo-make, do you think its worth adding as a build dep?
  • Test infra should ideally be different from the infra used for local development (ie uses a different sql db, runs on different ports, etc). The goal is someone should be able to run the server locally & be able to run tests simultaneously without them tainting each other. Can you explore spinning up 'test' infra distinct from the infra used to iterate locally? Happy for that work to be in another PR.
  • We should be able to run the entire test suite with a single command (ie at the moment its: cargo make db-up && cargo make full-test.

Finally, when I run the test suite locally, using cargo make db-up && cargo make full-test the suite gets stuck on "List of top 20 staps'. Does it succeed on your end? This is the top of the output I get with clean infra which I have to ctrl+c to stop:

Feature: List of top 20 snaps
  Scenario: Tails opens the store homepage, seeing the top snaps
   ✔> Given a snap with id "3Iwi803Tk3KQwyD6jFiAJdlq8MLgBIoD" gets 100 votes where 75 are upvotes
    > Given 25 test snaps gets between 150 and 200 votes, where 125 to 175 are upvotes
2024-02-21T22:05:16.153301Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.156976Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.207071Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.207803Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.229790Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.238972Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id

src/features/common/entities.rs Outdated Show resolved Hide resolved
@ZoopOTheGoop
Copy link
Contributor Author

ZoopOTheGoop commented Feb 21, 2024

Finally, when I run the test suite locally, using cargo make db-up && cargo make full-test the suite gets stuck on "List of top 20 staps'. Does it succeed on your end? This is the top of the output I get with clean infra which I have to ctrl+c to stop:

Feature: List of top 20 snaps
  Scenario: Tails opens the store homepage, seeing the top snaps
   ✔> Given a snap with id "3Iwi803Tk3KQwyD6jFiAJdlq8MLgBIoD" gets 100 votes where 75 are upvotes
    > Given 25 test snaps gets between 150 and 200 votes, where 125 to 175 are upvotes
2024-02-21T22:05:16.153301Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.156976Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.207071Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.207803Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.229790Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id
2024-02-21T22:05:16.238972Z  WARN ratings::features::user::use_cases: an error occurred when calling snapd: an error happened when attempting to retrieve info via /assertions/snap-declaration didn't find a snap with the given id

You have to be very patient with that test. If you want to see progress change the logging level to Debug. It takes a hot minute to run because of all the requests. (I think it printing the log isn't helping performance either). It took a while in the old tests too, but it's amped up here because I'm using more test snaps and Cucumber runs an isolated environment from scratch for each one. I might change parallelizing the vote generator to parallelizing the test snap initialization, doing both crashes it due to spawning too many tasks, but I think parallelizing against a coarser grain might help a little.

The warnings are going to happen no matter what, since we're randomly generating Snap IDs, any request to snapd to look it up is going to fail, because they're not real IDs. The actual program is set up to handle this (hence why it's only at Warn level, it's a problem in production but expected in tests). I'd recommend changing the log level to Error for just the server or else running the actual server in another tab/tmux/etc if you want to reduce noise, because otherwise you will get a few thousand of those messages. This happened on the old integration tests as well after introducing the category stuff in #70.

@ZoopOTheGoop
Copy link
Contributor Author

ZoopOTheGoop commented Feb 21, 2024

Re: infra, I don't think it's good as a given, but could just be put in main before the tests as a helper function.

Re: cargo-make I don't actually use it. I've been running with three tabs: dotenv -f test.env podman-compose up --force-recreate (I just don't like Docker because of its effects on my network adapters, does the same thing); dotenv -f test.env cargo run --release; and cargo test [--release]. So I'll look into the cargo-make bit, didn't realize we were relying on that.

Re: auto-starting infra, ahead of you in making that an issue :) #82, def a separate PR IMO.
Re: separate test infra. Introducing test.env is the first step to doing this, but #84.

@ZoopOTheGoop ZoopOTheGoop mentioned this pull request Feb 21, 2024
@tim-hm
Copy link
Contributor

tim-hm commented Feb 22, 2024

Thanks for the responses.

Agreed main sounds like a better place to checking if the infra is ready.

For cargo-make my focus there is for other people who are new to the project and aren't as familiar with how to get everything up and running. The idea being we can say here's a single command to run the test suite or here's a single command to iterate locally. We've provided the default happy path and then individuals can choose to go off piste.

Copy link
Contributor

@tim-hm tim-hm left a comment

Choose a reason for hiding this comment

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

Feel free to merge when you're ready

@ZoopOTheGoop ZoopOTheGoop merged commit 843aff5 into dev Feb 22, 2024
2 of 3 checks passed
@ZoopOTheGoop ZoopOTheGoop deleted the cucumber-tests branch February 22, 2024 17:07
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.

test: migrate integration test to cucumber
2 participants