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

Don't run unit tests on npm version #2208

Merged
merged 1 commit into from
Apr 23, 2021
Merged

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Apr 23, 2021

Currently, whenever we run npm version ... to create a release, the unit tests are run. This wastes time, and also requires the local dev env to be properly set up, which isn't really necessary to create a tag.

Passing unit tests doesn't mean the commit is good, as you need to run e2e tests as well. Also, before we tag a commit on master, it has to have passed a) in the PR CI checks; and b) when it lands on master; and c) when we build it for staging. If we've let a failed commit make it all the way through those stages, something is wrong already.

@humphd humphd added developer experience Helping the Developer Experience area: deployment Production or Staging deployment labels Apr 23, 2021
@humphd humphd added this to the 2.0 Release milestone Apr 23, 2021
@humphd humphd self-assigned this Apr 23, 2021
@birtony
Copy link
Contributor

birtony commented Apr 23, 2021

I don't know how I feel about it. This will make it super easy to create and push tags for broken commits. We will then have to remove those tags from remote repo. I would keep it, personally.

@humphd
Copy link
Contributor Author

humphd commented Apr 23, 2021

If e2e tests don't pass, you are already doing this. Also, why are we tagging a broken commit at all? The fact that it's already merged on master should mean it's good, or it shouldn't be there in the first place.

@birtony
Copy link
Contributor

birtony commented Apr 23, 2021

Can't disagree about the master branch. But what if the developer who is doing the release has a dirty tree on their machine? The tag is created locally and that's the reason why we have the tests running locally before the tag is created - to avoid creating a tag for broken commit. I agree that ideally such scenario should not happen, but we are not all senior devs here either...

@humphd
Copy link
Contributor Author

humphd commented Apr 23, 2021

Let's say that we want to ship ff0d814 as version 2.0.0. We've reviewed it (it's passed CI), and we've landed it on master, where it's also passed CI. Not only that, but it's passed CI on 3 different platforms and 2 different versions of node.js per platform twice. If a developer pulls that commit and tags it, it makes no difference if their machine is working or not because we aren't evaluating the quality of that commit when we tag; nor are we altering it in anyway. We are simply tagging a commit that is known to already be good. I'm not sure I see how the state of the developer's machine can impact things in anyway here.

Also, re: senior vs. junior devs, I understand your concern. I think we deal with this through limiting the people with power to make this happen.

Copy link
Contributor

@birtony birtony left a comment

Choose a reason for hiding this comment

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

@humphd fair points, let's try it then 👍🏼

Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

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

+1 on that write up

@humphd humphd merged commit 722a250 into Seneca-CDOT:master Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: deployment Production or Staging deployment developer experience Helping the Developer Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants