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

[WIP] Move Integration Tests to NodeJS #194

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

4141done
Copy link
Collaborator

Note

This PR is a work in progress but feedback on direction is welcome.

What

This PR moves the existing integration test suite into a Jest + Superagent-based suite.
The goal is to mostly replicate the behavior of the current test suite while also accounting for the large number of potential configurations for the project.

Details

Organization

  1. A basic test set handling encoding, basic usage, etc.
  2. Use-case based tests handling (subject to change)
    1. File/directory browsing
    2. Caching specific
    3. Single page application/asset server
    4. object path acrobatics

Design

The basic execution flow for each test will be:

  1. Ensure a clean state in Minio
  2. Create a bucket using the Minio Client
  3. Write the required test files to the bucket
  4. Build and start an s3-gateway container using the supplied settings for the scenario
  5. Use supertest to send and verify requests against the running container.
  6. Clean up the Minio bucket.

The principles we will try to follow are:

  • Test file structure specific to each use-case based test suite
  • Starting a general transition to thinking about use cases rather than individual config (although that will always be an option)
  • Decoupling general container build from integration test run

Copy link

@ciroque ciroque left a comment

Choose a reason for hiding this comment

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

Definitely a fan of moving away from shell-based test suites. This seems logical and approachable. Curious what the Contributor Journey will be.

Do new tests go in the __tests__/basic.test.js file, or new files based on the functionality being tests? I'm guessing new files. If that's correct, will there he a template or generator for creating new test files?

Overall this looks good. I have not written Jest / JavaScript tests in a long time so details are new again for me. Regardless, this is a good start!

Copy link

@ciroque ciroque left a comment

Choose a reason for hiding this comment

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

LGTM.

There are two options here:
1. In the `afterAll` block, comment out the code that runs the container teardown. After the test fails, you can run `docker logs -f nginx-s3-gateway-test-base`

2. Before the code that is erroring, add `timeout(3000)` then quickly run `docker logs -f nginx-s3-gateway-test-base` in another tab after the container starts.
Copy link

Choose a reason for hiding this comment

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

Maybe something like this: How to redirect Docker logs to file?, or using a mount to keep the logs around after the container closes?

tsconfig.json Outdated
Copy link

Choose a reason for hiding this comment

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

For me all these commented lines are quite distracting. Though I cannot imagine I'd be in this file very often.

@dekobon
Copy link
Collaborator

dekobon commented Jan 12, 2024

I would do an explicit call out for the need to install node and/or npm modules.

@4141done 4141done changed the base branch from master to main April 25, 2024 21:39
@4141done 4141done force-pushed the je-js-integration-tests branch from 5b21005 to abff552 Compare April 25, 2024 21:42
@alessfg alessfg force-pushed the main branch 2 times, most recently from 6d2bced to 119c0e0 Compare April 26, 2024 21:15
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.

3 participants