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

Improve tests by @mikejpeters #144

Merged
merged 4 commits into from
Feb 26, 2022
Merged

Improve tests by @mikejpeters #144

merged 4 commits into from
Feb 26, 2022

Conversation

Shereef
Copy link
Collaborator

@Shereef Shereef commented Feb 26, 2022

Background

Originally #143
From @fernando-mc:

The testing story right now is admittedly pretty terrible. It requires running a bunch of commands against the CLI and then checking that they’re working properly by looking at the expected location of the deployed site along with expected configuration values.

It’s hard to set them up and annoying to run them.

They’re not run on PRs right now. It’s possible they could be run after merging into the repo to avoid the issues with automated PR exploits.

The aws permissions required to test this (basically full s3 permissions) are potentially sensitive but I have them isolated in their own account. I’m okay with it running on that after approved merges by a maintainer but not automatically from PRs which I’m a little more concerned about.

Proposed changes

  • use runServerless util for all tests
    https://github.com/serverless/serverless/blob/main/test/README.md
  • separate unit & integration tests
  • update GitHub actions to run unit tests on push & pull requests
  • use @serverless/utils for confirm so it can be easily mocked
  • improved tests by @mikejpeters
  • added aws sso specific env var to whitelist
  • upgraded upgradeable dependencies except node-fetch because it had breaking changes

Proposed reviewers (optional)

@mikejpeters made the original change
@Shereef just adding onto it

This is currently blocked by #145

@Shereef Shereef changed the title Tests Improve tests by @mikejpeters Feb 26, 2022
mikejpeters and others added 3 commits February 26, 2022 04:08
- use runServerless util for all tests
  https://github.com/serverless/serverless/blob/main/test/README.md
- separate unit & integration tests
- update GitHub actions to run unit tests on push & pull requests
- use @serverless/utils for confirm so it can be easily mocked
@mikejpeters
Copy link
Collaborator

Your additions look good to me :)

On a side note I'm confused why you merged my PR into this branch instead of into master. Is this a good practice? I think it gets confusing because now you've effectively taken over the feature, and it's also possible for you to force push changes to my commit without any way (that I know of) for me to see a diff between what you've done and my original commit.

@Shereef
Copy link
Collaborator Author

Shereef commented Feb 26, 2022

@mikejpeters I didn't have push access to your repo and I wanted to help

Next time I'll ask first apologies if I offended you

@Shereef
Copy link
Collaborator Author

Shereef commented Feb 26, 2022

I've had a couple of open source contributions in the past where the maintainer wanted to make changes and did the same

I didn't think much of it and thought it was ok

Apparently you don't and I respect that

Co-authored-by: Mike <mikejpeters@gmail.com>
@mikejpeters
Copy link
Collaborator

Thanks for the explanation, I'm kinda new to collaborating on GitHub so was mostly just trying to understand. If that's best practice I'll go along with it

@Shereef
Copy link
Collaborator Author

Shereef commented Feb 26, 2022

I honestly don't know it only happened to me once and I only have like 4 open source contributions 😂

@Shereef
Copy link
Collaborator Author

Shereef commented Feb 26, 2022

We are now waiting on #145 to be resolved then we can merge this

@johnzoet
Copy link

I have tested "serverless-finch": "^3.0.0".
This version solves issue 135 which was marked as a duplicate of issue 132.
The version with issue 135 was "serverless-finch": "^2.8.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.

None yet

3 participants