Skip to content

Split out IntegrationTests into separate testTarget #157

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

Merged
merged 1 commit into from
May 10, 2021

Conversation

fabianfett
Copy link
Collaborator

@fabianfett fabianfett commented May 10, 2021

This PR splits out all the integration tests into a separate test target. This makes a number of things easier in day to day development:

  1. Unit tests can be run without setting up a test database.
  2. We can observe the test coverage of our Unit tests only.
  3. This allows us to decouple testing the language vs. testing the functionality against a database.

I think after this change we could change CI to:

Unit testing for:

      matrix:
        swiftver:
          - swift:5.2
          - swift:5.3
          - swift:5.4
          - swiftlang:swift/5.5-nightly
          - swiftlang:swift/main-nightly
        swiftos:
          - focal

Integration Testing tests for:

      matrix:
        dbimage:
          - postgres:13
          - postgres:12
          - postgres:11
        dbauth:
          - trust
          - md5
          - scram-sha-256
        swiftver:
          - swift:5.4
        swiftos:
          - focal

@fabianfett fabianfett requested review from gwynne and 0xTim May 10, 2021 08:21
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Seems like a very good idea to me. Looks good aside from two nits.

Comment on lines +60 to +62
if let boolValue = Bool(rawValue) { return boolValue }
if let intValue = Int(rawValue) { return intValue == 1 }
return rawValue.lowercased() == "yes"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might as well pull this out into a separate function given it's used below as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is previous code just copy and pasted here. If I try to refactor this we end up with a warning. Would love to keep as is for now. We should refactor the perf tests to allocation tests anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me to leave this for now. Out of curiosity, why do we end up with a warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"This code is never reached" for debug builds.

@fabianfett fabianfett force-pushed the ff-split-out-integration-tests branch from f7d6fb3 to b3820c0 Compare May 10, 2021 10:13
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

This LGTM but you haven't changed any of the CI setup - are you planning to?

@fabianfett fabianfett merged commit 290f3c1 into vapor:main May 10, 2021
@fabianfett fabianfett deleted the ff-split-out-integration-tests branch May 10, 2021 16:54
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