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

ci: Add integration tests for Parse Server #224

Merged
merged 33 commits into from
Oct 22, 2024

Conversation

vahidalizad
Copy link
Contributor

@vahidalizad vahidalizad commented Oct 13, 2024

Summary

This PR introduces integration tests for the S3 adapter in Parse Server, leveraging MongoDB and mongodb-runner to ensure the adapter's functionality with file handling. Additionally, a new npm command test:integration has been added to streamline the process of running these tests by automatically spinning up a MongoDB instance.

It's important to note that without running the tests using test:integration, some integration tests may fail due to the absence of a MongoDB instance. This might require minor adjustments to the pipelines to incorporate this new command.

I also considered removing the use of the config dependency in this setup, as it adds no significant value; the environment is strictly for testing in this repo, and we could simply pass it as an object. However, I haven’t implemented this change yet in this PR.

Closes: #222

Changes

  • Added integration tests for the S3 adapter in Parse Server.
  • Integrated mongodb-runner to handle MongoDB instances during the test process.
  • Added an npm script test:integration to run integration tests with MongoDB automatically.
  • Highlighted the potential need for pipeline adjustments to ensure the tests run correctly with test:integration.

Suggestion

Since we are making updates to the testing pipelines, I suggest we also replace the istanbul dependency with nyc for cleaner coverage reporting and to update our dependencies. I haven’t made this change in this PR, but it's a simple update that could improve maintainability.

Copy link

parse-github-assistant bot commented Oct 13, 2024

Thanks for opening this pull request!

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! The CI is failing, see https://github.com/parse-community/parse-server-s3-adapter/actions/runs/11315014989/job/31492994021?pr=224

I've left some comments below to simplify the test setup. It's also since we'll likely replicate this setup to other adapters in the future, so we'd want it as simple as possible.

package.json Outdated Show resolved Hide resolved
spec/mocks/MockEmailAdapterWithOptions.js Outdated Show resolved Hide resolved
spec/mocks/server.js Outdated Show resolved Hide resolved
spec/mocks/server.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mtrezza mtrezza changed the title test: Add Integration Tests test: Add integration tests Oct 14, 2024
@vahidalizad
Copy link
Contributor Author

vahidalizad commented Oct 16, 2024

Thanks for picking this up! The CI is failing, see https://github.com/parse-community/parse-server-s3-adapter/actions/runs/11315014989/job/31492994021?pr=224

I've left some comments below to simplify the test setup. It's also since we'll likely replicate this setup to other adapters in the future, so we'd want it as simple as possible.

I’ve addressed all the issues raised in the review and responded to the comments. I’ve also simplified the setup as much as possible, keeping future adapter replication in mind.

I’m not exactly sure what’s going wrong with Istanbul—it’s been giving me random errors on my machine too. On Windows, I get the error: @ECHO off SyntaxError: Invalid or unexpected token, and on Linux, there are issues with Parse Server Redis, similar to what’s happening in the CI.

I tried switching from Istanbul to NYC locally, and it had no problems handling the Parse Server integration tests. Maybe we should consider switching to NYC and then fix the pipeline? Also, we need to collaborate on getting MongoDB to run automatically in the pipeline. I’ve added a command in package.json to run MongoDB using mongodb-runner, but I haven’t updated the pipeline commands yet. Let’s work together on these changes to get everything running smoothly.

@mtrezza
Copy link
Member

mtrezza commented Oct 17, 2024

Sure, I'll try to open a separate PR to move to nyc, but it won't be before next week. Or do you want to do that and I look at mongorunner?

@vahidalizad
Copy link
Contributor Author

Sure, I'll try to open a separate PR to move to nyc, but it won't be before next week. Or do you want to do that and I look at mongorunner?

Unfortunately, my schedule is getting busier, but I’ll try to create the PR over the weekend. That way, you can check out MongoRunner and the rest of the pipeline.

@mtrezza
Copy link
Member

mtrezza commented Oct 21, 2024

@vahidalizad I've merged #225; could you update this PR and resolve the conflict? I'll then take a look at the mongo runner.

@vahidalizad
Copy link
Contributor Author

I’ve updated the PR and resolved the conflicts. However, I believe we still need to fix the MongoDB runner. I’ve created a new command, test:integration, and I think updating the test command might be enough. Could you take a look at it and see if that works?

.nycrc Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2024

I don't think there is a need to distinguish between integration test and other tests. Running npm test should run all tests, for simplicity. That also helps contributors to run all tests when they make any changes, and tests can be converted to integration tests without having to move them to a different file. In that sense, all the server management that is done in the integration.spec.js should be moved into the helper.js file of jasmine.

I'll take a look at the mongo runner.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.28%. Comparing base (0e0d721) to head (fe0e6ab).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   95.60%   95.28%   -0.33%     
==========================================
  Files           2        2              
  Lines         205      191      -14     
  Branches       44        0      -44     
==========================================
- Hits          196      182      -14     
  Misses          9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2024

I've refactored the server integration; the CI passes, if we could remove the conformance tests as a last step, then I think we should be done, see discussion in #224 (comment).

@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2024

I have removed the conformance tests because of several issues:

  • They are pulled from a separate repository that is not part of Parse Server. That repo would need to be maintained and kept in sync with Parse Server.
  • They tests basic Parse Server storage adapter APIs, but the Parse Server integration substitutes that and is more robust because it allows to test against specific versions of Parse Server.

The CI now explicitly tests against Parse Server 6 and 7. The integration.spec.js contains a test section for Parse Server <=7:

describe_only_parse_server_version('<=7')('Parse Server <=7 integration test', () => {

In case the interface changes in newer Parse Server versions, it can have its own integration test group.

@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2024

@vahidalizad What do you think about the PR, is it ready to merge?

@vahidalizad
Copy link
Contributor Author

@vahidalizad What do you think about the PR, is it ready to merge?

Yes, I think we’re all set! Thanks for doing a great job with all the cleanup and everything else. Much appreciated!

@mtrezza mtrezza changed the title test: Add integration tests ci: Add integration tests for Parse Server Oct 22, 2024
@mtrezza mtrezza merged commit 1f74921 into parse-community:master Oct 22, 2024
9 of 10 checks passed
@vahidalizad vahidalizad deleted the integration-test branch October 22, 2024 15:07
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Parse Server integration tests
3 participants