Require multiple editor approvals to merge unknown files #105
Conversation
|
3 test cases had to be deleted due to them being out-of-date. |
|
@MicahZoltu @axic ready for review. |
|
This does not appear to fix #53 so I removed that from the OP (so it doesn't get auto-closed). Also, I would like to better understand why we are removing some of these tests. We also should test this on a fork to make sure it behaves as expected, and possibly add one or more new test cases to cover its behavior. |
They all dealt with approving unknown files with one editor's approval (specifically, the Gemfile).
I've already done that :) (https://github.com/ethereum/EIP-Bot/pull/105/files#diff-b6ddeb8623cdd627ccf86b7b69fb769923e00d0a27fb35869b71492a6027815a) |
|
I can confirm that it does not merge with a single editor approval, as f2e6aa2 didn't pass. |
|
I've now added test cases describing every possible condition. If this passes, it is ready to merge. |
|
Since we don't have any active maintainers working on the bot at the moment (we are all EIP bot newbs), I would like to see this manually E2E tested before we merge, just to make sure we are generally doing things right. I believe @JEAlfonsoP is working on doing E2E tests on their fork, perhaps they could test this change (probably easier than testing the many-files change)? @JEAlfonsoP to this this, update your EIP fork's workflow to point at the latest commit of this branch and then create a PR that touches a non-EIP file and the bot should: |
|
B implies C unless branch protection is set up incorrectly. Also, I have added E2E tests! |
|
I can do that. |
|
Workflow is setup to run with this commit. |
|
I have indeed added E2E tests for all cases (1 author approval, 1 editor approval, and 2 editor approvals) and it passes all of them. |
|
Didn't mean to close, oops. |
|
I wouldn't consider the automated tests we have to be E2E. They are either unit tests or functional tests (people disagree on what this class of tests is called). E2E usually means you are testing the entire system end to end, which includes real versions of every piece of software being interacted with. I don't think there is a way to automate this with GitHub, which is why I was hoping that @JEAlfonsoP could do the end to end test manually. |
Do you have a link to where you ran these tests (the GH action runs)? The test to do is the one I described above, did it pass all of the various lettered items in that list? |
|
I will be more than willing to do these tests once #109 is merged since I have a working test environment. |
| @@ -108,7 +108,7 @@ export class RequireFilenameEIPNum implements IRequireFilenameEIPNum { | |||
| const approvals = await this.getApprovals(); | |||
|
|
|||
| const isEditorApproved = | |||
| _.intersection(editorApprovals, approvals).length !== 0; | |||
| _.intersection(editorApprovals, approvals).length >= 2; | |||
There was a problem hiding this comment.
FYI @MicahZoltu - this is the only functional change made. All the rest is fixing the test cases.
Pandapip1
changed the title
|
This is going to get tested manually anyways. Merging. |
Fixes #46, Fixes #79