Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

SEP 10 - PR Merge Requirements #13

Merged
merged 6 commits into from
Sep 4, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions 0000-pr-merge-requirements.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
- Feature Name: PR Merge Requirements
- Start Date: 2019-05-21
- SEP Status: Draft
- SEP PR: (leave this empty)
- Salt Issue: (leave this empty)

# Summary
[summary]: #summary

Lay out the requirements to merge a PR

# Motivation
[motivation]: #motivation

As we begin our working groups, to make sure that our community is all on the same page, we want to
clearly define our requirements to merge a PR. This should benefit developers, reviewers and those
merging the PRs.

# Design
[design]: #detailed-design

All PR requirements
- Approval Reviews: 1 approval review from core team member OR
1 approval review from captain of working group
Copy link

Choose a reason for hiding this comment

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

To avoid bottleneck, do make sense that two +1 from team members is also OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you clarify this statement? Do you mean you want to require three approvals? currently as it is written this will require 1 approval for PRs unless we want to add an exception for tests not passing or test not written which will require 3.

Copy link

Choose a reason for hiding this comment

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

Ah sorry. No. What I mean is that the +1 from the captain of the working group can be replaced with two +1 of other team members, if there is not any -1 from the captain or any other group member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for clarifying. We talked about this extensively and the reasoning for requiring the core team member or the captain was since the captain of the working group will be shepherding a particular feature set during a release we want to ensure they also have eyes on the changes coming in to ensure the team is progressing as expected. but i would love to see what the @saltstack/team-core thinks about this idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like what you're after is just making sure that it doesn't take forever to get stuff merged in. That's a fair desire, and I think at least in part what's driving the idea behind working groups.

I think largely it's a question of how diligent approvers are, and maybe one of trust? On the one extreme, we just trust everyone to write correct code and push it willy-nilly. On the other extreme we have one single individual who's merging code (see: Linux Kernel), and it doesn't make it in without intense scrutiny.

Personally, when I'm doing a PR the longest part of that task is actually just understanding what the change is supposed to be, and then in some cases running the code myself. Also checking for things like, "Does this even make sense??"

Of course, it's hard to tell if other people mean the same thing by that check mark. Then there's the whole idea of just getting more eyes on the code. I've found typos that other people have missed, and vice versa. Heck, sometimes I've come back and reviewed my own code and discovered problems that I missed!

I don't think that I have a strong feeling about what is/should be the right answer here. I have noticed (like yourself) that sometimes it takes too long to get something merged in. I think the biggest problem there is actually the one that we're addressing head-on: getting our testing green and reliable and requiring it to stay green before merge, even if the failure appears unrelated to the code at hand.

By doing that it's taking us longer initially, but as we make progress and fix those issues we will have more confidence in the test suite, and it will run faster. That means there's less time for other changes to get merged in causing conflicts and holding up the merge. So maybe that will fix the problem.

Or maybe we'll find out that we're still having issues merging fast enough.

I think for now my recommendation is that we err on the side of caution - keep the merge policy as is, but for our first working group postmortem we should also figure out if we've got good friction there, or if we've just got stumbling blocks. I think a certain amount is healthy for finding bugs before they're released in the wild. But if we're just getting in the way of progress, without a payoff, I think we should address that.

Copy link

Choose a reason for hiding this comment

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

It sounds like what you're after is just making sure that it doesn't take forever to get stuff merged in.

You are correct. There are PR quite old in the backlog. But my main concerns are pointing also in another direction:

  • People have vacations, gets sick, live gets complicated, etc. If the captain is in this situation, only a core member can review the PR for this team. And I am not sure that this will scale.

  • The captain is part of a team. If only one component of the team is authoritative in reviews, there is no motivation for the rest of the team to review anything.

  • The captain sometimes will not be an expert in all the PR, so she/he can delegate some PR to other members.

I think largely it's a question of how diligent approvers are, and maybe one of trust? On the one extreme, we just trust everyone to write correct code and push it willy-nilly.

I would assume that we are professionals, knowledgeable and with good intentions. The review process is mostly to be sure that the quality standard is there, and no mistakes are slipped.

On the other extreme we have one single individual who's merging code (see: Linux Kernel), and it doesn't make it in without intense scrutiny.

AFAIK the Linux kernel is managed by a big team, Linus trust the team when merge into his authoritative repo. This is basically why git is a distributed source code manager, because he cannot scale.

Personally, when I'm doing a PR the longest part of that task is actually just understanding what the change is supposed to be, and then in some cases running the code myself. Also checking for things like, "Does this even make sense??"

You mean during the review? If so this is a good approach indeed and very valuable.

Of course, it's hard to tell if other people mean the same thing by that check mark. Then there's the whole idea of just getting more eyes on the code. I've found typos that other people have missed, and vice versa. Heck, sometimes I've come back and reviewed my own code and discovered problems that I missed!

Indeed.

I don't think that I have a strong feeling about what is/should be the right answer here. I have noticed (like yourself) that sometimes it takes too long to get something merged in. I think the biggest problem there is actually the one that we're addressing head-on: getting our testing green and reliable and requiring it to stay green before merge, even if the failure appears unrelated to the code at hand.

I very much agree here.

By doing that it's taking us longer initially, but as we make progress and fix those issues we will have more confidence in the test suite, and it will run faster. That means there's less time for other changes to get merged in causing conflicts and holding up the merge. So maybe that will fix the problem.

Ah I see. Do you think that the CI is the main reason of slower merge rate, and not by the lack of reviewers?

Or maybe we'll find out that we're still having issues merging fast enough.

I think for now my recommendation is that we err on the side of caution - keep the merge policy as is, but for our first working group postmortem we should also figure out if we've got good friction there, or if we've just got stumbling blocks. I think a certain amount is healthy for finding bugs before they're released in the wild. But if we're just getting in the way of progress, without a payoff, I think we should address that.

I agree to review any decision done, and learn from the experience.

And I understand the difficult of trusting external reviewers. IIUC the captain can be also a community member, if so the proposal in this document is in fact a way to open more the project that how it is today, and this is very valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • People have vacations, gets sick, live gets complicated, etc. If the captain is in this situation, only a core member can review the PR for this team.

There is a provision in the working group SEP (see #12) where the captains can delegate the responsibility.

And I am not sure that this will scale.

I'm sure you're right 😃

I do expect us to learn through this process, and improve by applying the lessons that we learn. If these working groups end out taking off with a lot more interest and velocity, that would be great! Probably a whole new class of problems that we'll have to face.

  • The captain is part of a team. If only one component of the team is authoritative in reviews, there is no motivation for the rest of the team to review anything.

I hope that's not seen as the case - I would review code whether or not my review was the gate for merging for the same reason that I mentioned earlier. Sometimes I manage to catch problems that other reviewers miss. The main reason that I review code is to catch defects earlier and more cheaply. I think that's a worthwhile attitude to cultivate on our teams.

  • The captain sometimes will not be an expert in all the PR, so she/he can delegate some PR to other members.

AFAICT anyone can still review PRs, requesting changes, or marking their approval. In these situations the captain should request that one of the other team (also) reviews the code.

I would assume that we are professionals, knowledgeable and with good intentions

I trust that you and I are 😄 I'm also paranoid about both careless and bad actors. My threat model is that someone, at some point, either because of a personal or political agenda will make an attempt to compromise the security of the Salt project because it is a high-value target. Heck, I'm not above suspecting that I could become compromised in some way. I'd like to believe that I never could be... but better to design the process with the highest chance of catching those attempts, intentional or otherwise

Ah I see. Do you think that the CI is the main reason of slower merge rate, and not by the lack of reviewers?

I do. I can't speak for everyone else on the team, but I've had review processes that look like this:

  • Review the code. Looks great.
  • Tests are still running. Leave browser open and go do other work.
  • Come back some time later and discover that the tests were green but the branch has been merged to. Drat!
  • Push a merge to the PR branch
  • Tests are still running. Leave browser open and go do other work.
  • Come back some time later to discover new test failures. Either fix it or relaunch build.
  • Come back some time later and discover that the tests were green but the branch has been merge to. Drat!
  • Repeat 2-3 times
  • Finally test is green and branch is up to date
  • merge

Having stable branches/tests should drastically improve this process. Especially if we can get a reliable functional/integration layer setup that we can run instead on most PRs. Right now our "integration" tests pretty much exercise the full salt, including the network, which is probably not necessary. If our tests just ran in seconds or minutes, we would be able to merge much faster!

And I understand the difficult of trusting external reviewers. IIUC the captain can be also a community member, if so the proposal in this document is in fact a way to open more the project that how it is today, and this is very valuable.

Yes, they can. We're working on opening things a lot more, which I'm super excited for!

- All tests pass.
Copy link

Choose a reason for hiding this comment

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

Some windows tests and some pylint are bit fragile. Also the CI sometimes fail because other random reasons.

I support this requirement: we cannot merge with all the tests passing, but I wonder what can we do about the cases where CI is not ready. Is there any mechanism where the contributor can help here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is a case where we are okay with the tests not passing due to CI issues this would fall under the exception to require 3 reviews.

Is there any mechanism where the contributor can help here?

If there is a test that is failing that is not related to the PR you can definitely dive in and figure out why and push a separate PR to try to fix, but ultimately that responsibility lays on our team. If windows or pylint tests are failing constantly we need to dive in and figure out why. I think it makes sense to add some more documentation around how we have these tests setup so its easier for contributors to try to run them with the same infrastructure as us. I'll add the step to document this to this SEP.

Copy link

Choose a reason for hiding this comment

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

I think it makes sense to add some more documentation around how we have these tests setup so its easier for contributors to try to run them with the same infrastructure as us. I'll add the step to document this to this SEP.

Thanks, I love that.

- Cannot merge your own PR until 1 reviewer approves from defined list above that is not the author.

Bug Fix PR requirements:
- Test Coverage: regression test written to cover bug fix.
Copy link

Choose a reason for hiding this comment

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

Do makes sense to create always an github issue and reference the issue number in the comment?

This will eventually make easy to track the status of the bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i like that idea :)

- Point to the issue the PR is resolving. If there is not an issue one will need to be created.

Feature PR requirements:
- Test Coverage: tests written to cover new feature.
Copy link
Contributor

@cachedout cachedout May 31, 2019

Choose a reason for hiding this comment

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

(A wild @cachedout appears)

IMHO this is going to be very difficult to achieve for a large number of cases.

The first, and most substantial barrier to this is that new features are rarely built in isolation and instead, usually enhancements to features which already exist.

However, some of the existing features do not have test coverage. As such, a rule like this becomes extremely burdensome. Effectively, this means that if I want to add a function to a module which does not have test coverage, I'm suddenly tasked with creating a testing framework around that module which might be a task which is much larger than I'd prefer to take on -- especially in the case below where this policy seems to apply to bug fixes as well.

If you just look at unit test coverage for modules right now, this condition wherein a module exists but no unit test file is present would apply in just under half the cases. (Somewhere near 43% with rough math.)

This represents a substantial barrier to entry for new feature development. It does not seem to me to be a responsible policy to implement until such time as their is at least some basic testing in place for all modules. Is there a plan to achieve this at present?

I see below that there is an exemption for this policy which means that that PR is left open for three months but it's not clear to me what good that does. Could somebody please clarify?

Copy link

Choose a reason for hiding this comment

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

(A wild @cachedout appears)

IMHO this is going to be very difficult to achieve for a large number of cases.

The first, and most substantial barrier to this is that new features are rarely built in isolation and instead, usually enhancements to features which already exist.

I do not see the problem here. I extended some tests or created new ones when I changed the behavior of a current feature, or create a new one when the feature is new.

However, some of the existing features do not have test coverage. As such, a rule like this becomes extremely burdensome. Effectively, this means that if I want to add a function to a module which does not have test coverage, I'm suddenly tasked with creating a testing framework around that module which might be a task which is much larger than I'd prefer to take on -- especially in the case below where this policy seems to apply to bug fixes as well.

I think that this makes the rule even better. IMHO there is no excuse to improve the code in any commit. The poor state of the current coverage is not an excuse to make it even worse.

Another argument is also the quality of the current tests. Some of them are not in good shape either, but requiring to improve the current tests related with the PR under review can be a debatable option.

On the other hand, I interpret the new part of the sentence as that you only need to cover the new feature behavior, but not the old one. So this means that, strictly speaking, only a partial test could be OK.

This represents a substantial barrier to entry for new feature development. It does not seem to me to be a responsible policy to implement until such time as their is at least some basic testing in place for all modules. Is there a plan to achieve this at present?

Maybe is only me, but not forcing test coverage in something like salt is a very worrisome PoV. Executing code in production machines that is not tested by any unit test / integration test is an argument to not to trust this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Executing code in production machines that is not tested by any unit test / integration test is an argument to not to trust this code.

I agree. However, being unable to get reliable signaling from the tests themselves is a much greater liability. When tests are required, it puts contributors in the position of writing tests simply for the sake of writing tests. Too often, these tests are poorly constructed, unhelpful, and unmaintained.

In my view, it is much better to encourage tests, of course, but to give contributors the freedom to make tests when it makes sense to do so. A blanket policy removes the ability for contributors to make sensible decisions in this regard and there are many examples of tech debt accumulated in the project in this manner.

Copy link

Choose a reason for hiding this comment

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

Executing code in production machines that is not tested by any unit test / integration test is an argument to not to trust this code.

I agree. However, being unable to get reliable signaling from the tests themselves is a much greater liability. When tests are required, it puts contributors in the position of writing tests simply for the sake of writing tests. Too often, these tests are poorly constructed, unhelpful, and unmaintained.

But there are reviewers that can evaluate what the test is doing.

If some developer, for the sake of speed decide to do a test that is equivalent to:

result = module.function(params)
assert result == 'expectation' or True

a reviewer must be aware that this is useless and help to design a better test. And do not merge the code until this issue is fixed.

On the contrary, if the developer understand that the test is optional, so be it and no test will be done. IMHO this will not improve the situation.

In my view, it is much better to encourage tests, of course, but to give contributors the freedom to make tests when it makes sense to do so. A blanket policy removes the ability for contributors to make sensible decisions in this regard and there are many examples of tech debt accumulated in the project in this manner.

I do not see how making the tests optional will decrease the technical deb (sorry if I am misunderstanding your point)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey wild @cachedout and @aplanas! Thanks for the comments!

On the other hand, I interpret the new part of the sentence as that you only need to cover the new feature behavior, but not the old one. So this means that, strictly speaking, only a partial test could be OK.

That's the prevailing attitude. Having zero tests is not good. Having basic tests of some sort is kind of the MVP.


I agree. However, being unable to get reliable signaling from the tests themselves is a much greater liability. When tests are required, it puts contributors in the position of writing tests simply for the sake of writing tests. Too often, these tests are poorly constructed, unhelpful, and unmaintained.

But there are reviewers that can evaluate what the test is doing.

Precisely.

The purpose of the tests is to verify that 1) code is behaving as intended and 2) future code changes do not have unintended side effects.

The purpose of code reviews is so that humans can verify that the code is doing what it's supposed to do. After all, it's pretty trivial to write code that always passes tests, if you don't care what the tests or code is doing.

Too often, these tests are poorly constructed, unhelpful, and unmaintained.

I have encountered several of these tests 🙂

unmaintained

This is the part that we're trying to fix. Maybe it's worth updating this SEP to specify some of the human-oriented parts of this.

We're not requiring tests to satisfy some arbitrary notion that we should have test coverage, nor are we mandating that X style of tests are written, nor even X level of code coverage.

But we do expect that:

  1. If you're fixing a bug, you can write a test that exhibits the buggy behavior. And then write the fix for it.
  2. If you're adding a feature you should be able to codify how that feature is supposed to work. By spelling that out with an automated test 😄 It doesn't necessarily have to run the master-minion gauntlet, or really even hit the underlying system - though preferably the tests+code can show what happens when the system isn't in the "happy path" state.

What we've seen is that many, if not most, of our bugs can be traced back to ignoring test failures and merging code, even if it's (apparently) unrelated to the test failures. We're not quite at a point where we have full confidence in our tests, but that's what we're after. If there's a better way to communicate this in the SEP, I think we should consider updating the text.

@cachedout you're absolutely right that we shouldn't just require tests for testing's sake.

Do you have any suggestions for how we could better express what our goals are, and why we have that requirement?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I am choosing to call it out as a specific form of technical debt in which the debt becomes due at the point at which a test may be incorrectly reporting a working module when it turns out that the module is not functional from the perspective of the user.

I'm not sure that this is different at all from not having tests. The benefit of having tests, however, is that when frobnosticate upgrades from version 3 to 4, now we have tests for version 3, so when we make updates for version 4 and we inadvertently break code for version 3 we have a quick feedback showing that we did, in fact, break existing behavior.

It is honestly troubling that you see no distinction between the two. I'm not being hyperbolic here.

Scenario 1: Test exists which isn't actually testing the module, but rather the other logic in the function, with external dependencies mocked. The test passes because we've controlled for the operation of the module. Does it work? We don't know for sure, but the passing test lets us think that it is working.

Scenario 2: No test. Does it work? We have no idea.

When I say that I'm unclear on the distinction, what I mean is that I don't see why technical debt enters into the discussion.

The reason I say that is because the discussion is around this question:

What happens when system outside of Salt changes in a breaking way?

  • No tests
    • Salt breaks
  • Only tests that aren't an accurate reflection of the underlying system
    • Salt breaks

So why does technical debt about those tests enter the conversation? That's what I don't understand. Tests that don't use real 3rd party systems do not tell us anything about 3rd party systems. Is anyone arguing that they do?

These are not remotely the same. Sure, a unit test where you've mocked out the thing you're testing helps keep you from breaking logic with later changes to Salt, and that's not nothing. But it's also not a substitute for properly testing the operation of the module, and it doesn't help you to detect upstream changes.

Right. I think that's what everyone is saying.

So when the upstream module changes, you'd better hope it does so in a way that causes the mocks you wrote to no longer allow the test to pass, or else you're oblivious to a change that is meanwhile causing the module to break for the people actually using that module.

I don't think anyone is disagreeing with that statement either?

So yes, there's a cost to maintaining the tests, but it's not like the alternative is free. There's a cost there, too.

I don't think that anyone is arguing that having no tests isn't costly, so let's dispense with this straw-man argument, please.

What are we arguing about anyway? I just looked back through this discussion... ah. Okay, now I see. The question is about this statement:

Test Coverage: tests written to cover new feature.

And the question is "what good will this do?

The answer then, should be this:

  • Provide an increased confidence in the code in question
    • With accompanying tests, at least there is enough understanding of what the code should be doing that automated tests can be written
    • Spending some extra time writing tests may indicate more thought/care went into the design.
  • Unit tests provide increased confidence that code does not change in an unexpectedly backwards-incompatible fashion
  • Functional tests provide increased confidence that salt interactions with the underlying systems behave as expected
  • End-to-end/Integration tests provide increased confidence that Salt as a whole operates correctly.

Unit tests are just one part of a test infrastructure, arguably a minor part

Agreed.

, and the fact that they're easier to write will make them the go-to if we have a dogmatic policy to require tests instead of making the focus what it should be: to come up with the best way to accurately test how the software actually works.

🤷‍♂ That may be correct. But is it better to have no tests, or some tests? Because what we're talking about is the difference between merging code that has no tests, and merging code that has at least some tests. Or changing this SEP to be more explicit about test requirements.

My personal opinion is that currently the way it's stated is fine. My current experience says that some tests are better than no tests. And not spelling out exactly what kinds of tests we're requiring gives the community more flexibility - just want some unit tests? Cool, that's a great start. Want to create an entire set of tests, with unit, functional, and integration tests, as well as extensive documentation and examples? Awesome!

But that's just my strong opinion, weakly held.

I'd recommend watching this excellent talk from this year's PyCon: https://www.youtube.com/watch?v=Ldlz4V-UCFw

Nice! That definitely illustrates a lot of the challenges to proper testing, and I've definitely seen those problems myself. Hopefully created them less often 😝 I know that I definitely have a preference for the stub-style doubles and DI/functional style code/tests myself.

I know that we will probably get some PRs that have tests that mock too much, and some that will commit other anti-patterns. My hope is that we'll catch these during the PRs and can give people guidance on how to write better tests. I think this is a worthwhile first step that will produce a lot of good information that we can use as we work to improve our test infrastructure. I think it's a Goldilocks requirement - it's not too small and it's not too big, it's just right.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I say that is because the discussion is around this question:

What happens when system outside of Salt changes in a breaking way?

  • No tests
    • Salt breaks
  • Only tests that aren't an accurate reflection of the underlying system
    • Salt breaks

So why does technical debt about those tests enter the conversation? That's what I don't understand. Tests that don't use real 3rd party systems do not tell us anything about 3rd party systems. Is anyone arguing that they do?

No. For the umpteenth time, the concern (and an entirely valid one) is that this will lead to unit tests being over-represented due to their expediency. The technical debt comes from waiting to write better tests, or just not writing them at all.

Exhibit A:

% find tests/unit -type f -name test_\*.py | wc -l
726
% find tests/integration -type f -name test_\*.py | wc -l
224

This ratio is not going to get better if we mandate that tests are written without making it easier to write more accurate tests.

My current experience says that some tests are better than no tests.

This is reductive to the point of absurdity.

I know that we will probably get some PRs that have tests that mock too much, and some that will commit other anti-patterns. My hope is that we'll catch these during the PRs and can give people guidance on how to write better tests. I think this is a worthwhile first step that will produce a lot of good information that we can use as we work to improve our test infrastructure.

"Hope" is a particularly bad strategy on which to base the future of the test infra. You can "hope" all you like that we're going to use unit tests as a starting point and build around them, but as someone who is far more familiar with Salt and how we've approached testing over the years, I can say that this is not very likely. The tests that are written all too frequently just become "the tests" for that module, unless repeated bug reports cause us to write better ones.

A hard requirement like this is for after we've approached creating better ways to test complex things. Not before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This represents a substantial barrier to entry for new feature development. It does not seem to me to be a responsible policy to implement until such time as their is at least some basic testing in place for all modules. Is there a plan to achieve this at present?

I see below that there is an exemption for this policy which means that that PR is left open for three months but it's not clear to me what good that does. Could somebody please clarify?

Just want to go back to your original questions surrounding what happens when a PR is left open @cachedout

I do think that we should more clearly define how we would approach the situation where a user is not able to write a test and we do not want to merge without test coverage. One idea was to run community salt-sprints where we add tests for these specific PRs, but I think its fair to say that we should plan these out so they occur often, and are not just wishful thinking. These test focused salt-sprints could also slowly try to tackle this concern:

"If you just look at unit test coverage for modules right now, this condition wherein a module exists but no unit test file is present would apply in just under half the cases. (Somewhere near 43% with rough math.)"

What ya think of this approach? I'm sure you will point out something I'm missing ;)

Choose a reason for hiding this comment

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

This could work, my concern is that how do you make sure that the subject matter expert is in charge of making the tests for the module?

I could definitely see people writing tests for things that they don't understand and then all that is being done is writing tests for coverage purposes instead of for actually testing the important bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question. Maybe we ensure certain test coverage is assigned out to the appropriate work groups during the sprint? Although this only covers certain areas for now as we have only started up a couple work groups this time around.

I'm not sure how to ensure someone is a subject matter expert unless I'm familiar with them in the community, just the same as i do not know if they are an expert when they push a pull request. The only other thought I have is to make it clear in the announcement beforehand that these are the tests we are covering and the areas and if you have expertise in this area please join to help write tests. Also to make it clear the intention was also to include team-core in this sprint to help tackle these test issues as well so they should be able to help in some of those areas that others do not have expertise in.

I think another thing that will help is for our team to triage the PRs/tests beforehand and we can add a bit of direction on each PR on how someone could add adequate test coverage.

- Release Notes: Add note in release notes of new feature for relative release.
Copy link

Choose a reason for hiding this comment

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

Ouch, that is new for me. Where are the release notes file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc/topics/releases

Copy link

Choose a reason for hiding this comment

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

Thanks. Maybe is my fault, but I do not see this process documented.

If this is the case maybe we need to extend the page in saltstack that explain how to contribute, and add those pointers and the location of the releases file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing that out. I'll include that in the documentation when we document PR requirements in the contributor docs after this is merged.

- Add `.. versionadded:: <release>` to module's documentation
Copy link

Choose a reason for hiding this comment

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

How do we now the version when we are in develop branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is the next release that is due to release, for example right now it would be neon as you can see from here: https://docs.saltstack.com/en/latest/topics/releases/version_numbers.html

the person reviewing the PR should be correcting the version if it is not correct.

Copy link

Choose a reason for hiding this comment

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

So is OK to add .. versionadded:: TBD. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah thats a great idea, then the reviewer can clarify the version. I'll include that in the docs as well :)

Copy link

Choose a reason for hiding this comment

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

I like this TBD, also makes it easier for future release to grab all of those.
As the proces will be the same for every release instead of figuring out what you need code name wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to clarify the idea, the reviewer would inform the contributor what to change TBD to if they were not sure before we merge. We would not merge it with TBD, because we want to ensure the intended version is there before merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that someone would be merging a feature that wouldn't go into the next named release?

If we do require that, perhaps we could just have a simple text file in the project root like "NEXT_CODENAME" or something, so it's easy to find (and thus, use).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it would be easiest to update it here: https://docs.saltstack.com/en/latest/topics/releases/index.html since we already update it for releases, and it would be available in both the code base and the documentation online.


Exceptions:
- Changes that do not require writing tests: documentation, cosmetic changes (ex. log.debug -> log.trace),
fixing tests, pylint, changes outside of the salt directory.
- If an exception needs to be made around the requirement for tests passing or writing a test alongside
a bug or feature it requires 3 approvals before it can be merged.
- If a test is not included in a bug fix or feature and the author cannot write a test, keep the PR open
for 3 months with the “Needs Testcase” label.

Clarifications:
- Contributers only need to write test coverage for their specific changes.

Requirements for Merge Access:
- Two factor authentication is required for any user with merge access.

## Unresolved questions
[unresolved]: #unresolved-questions

- Will need to document this if approved and merged. Documentation needs to include everything specified in
this SEP and the following details:
* Document current test infrastructure and how contributors can mirror the test runs to help troubleshoot
test failures
* Clarify where the release notes are located and how to determine the version to include in versionadded.
If it is still not clear the contributor can include TBD and the reviewer would clarify the version.
* Document when integration tests are more applicable over unit tests.
* Document what test coverage still needs to be added.

# Drawbacks
[drawbacks]: #drawbacks

Requiring tests and ensuring all tests pass will slow down the PR merge process, but will provide more
stability in salt.