Cherry-picker: Remove BACKPORT_COMPLETE. UNSET state after finished. by Mariatta · Pull Request #315 · python/core-workflow

Mariatta · 2019-03-15T16:42:53Z

No description provided.

Mariatta · 2019-03-15T16:55:51Z

@webknjaz @asvetlov can you take a look. miss-islington is seeing the "BACKPORT_COMPLETE" status, but couldn't continue because it is not an allowed state. I think it should be an allowed state, or did you have different expectation.

webknjaz · 2019-03-15T16:58:01Z

Checking it now.

webknjaz · 2019-03-15T17:04:30Z

@Mariatta I expected that state would be reset if everything is completed successfully. How about calling reset_state() after line 391 instead?

webknjaz · 2019-03-15T17:05:17Z

Any non-allowed states are supposed idicate that something went wrong.

Mariatta · 2019-03-15T17:15:58Z

Ok I've removed BACKPORT_COMPLETE. unset will be called instead, and using the UNSET state.

Mariatta · 2019-03-15T17:38:08Z

uugh don't know what's the deal with travis 😟 Tests-passing-locally-for-me™️

webknjaz · 2019-03-15T18:21:50Z

Oh, that's weird... Did you run all tests locally or just related?

Mariatta · 2019-03-15T18:40:21Z

Hmm I ran everything pytest cherry_picker/test.py -v in the cherry_picker directory

webknjaz · 2019-03-15T18:43:58Z

I checked this branch locally as well and it passes.

webknjaz · 2019-03-15T18:45:50Z

@Mariatta just to be on the safe side, could you please clear the cache in Travis CI? It's here https://travis-ci.org/python/core-workflow/caches

Mariatta · 2019-03-15T18:53:35Z

I've cleared cache and restart the build, still no luck 🤕 Anyone else can try this locally?

black-out · 2019-03-15T19:21:31Z

🐍🌚🤖 @Mariatta, I've formatted these files using black:

cherry_picker/cherry_picker/cherry_picker.py
cherry_picker/cherry_picker/test.py
(I'm a bot 🤖)

webknjaz · 2019-03-15T21:24:01Z

@Mariatta could you please merge #311 and then rebase this one? The test suite didn't use new fixtures in some tests and it could improve the situation, I think (hope).

webknjaz · 2019-03-15T22:34:41Z

@Mariatta I know what the problem is! 🎉

So git log -r dc896437c8efe5a4a5dfa50218b7a6dc0cbe2598 is executed to check the repo.
But Travis CI only gets 50 last commits by default, not the whole tree. So this commit is now further than 50 commits and the check fails.

webknjaz · 2019-03-15T22:37:27Z

Solutions:

Change the sha in config to something newer
Find &install-and-test-cherry-picker on line 28 in .travis.yml and extend the mapping to contain Git config adjustment:

- &install-and-test-cherry-picker...git:
depth: 200...

black-out · 2019-03-16T19:51:01Z

🐍🌚🤖 @Mariatta, I've formatted these files using black:

cherry_picker/cherry_picker/test.py
(I'm a bot 🤖)

webknjaz · 2019-03-17T22:22:28Z

+ ],
+)
+@mock.patch("os.path.exists")
+@mock.patch("cherry_picker.cherry_picker.validate_sha")


These tests aren't testing validate_sha itself, but the other functionalities. I think it is ok to be mocked.

I disagree: more integrated tests is better. Testing a combination of things makes it all more robust. Especially when it costs nothing.
As in

webknjaz

I think that mocking validate_sha is wrong. This way you'll just completely disable testing of the specific code path. It doesn't cure the root of the issue but only symptoms. Along with that, it makes the project vulnerable to regressions.

Mariatta · 2019-03-17T22:38:01Z

hm, but changing it to recent sha also only delays the problem, same issue will happen again in the future.

black-out · 2019-03-17T22:38:27Z

🐍🌚🤖 @Mariatta, I've formatted these files using black:

cherry_picker/cherry_picker/test.py
(I'm a bot 🤖)

webknjaz · 2019-03-17T22:40:36Z

@Mariatta I think you should just make git clone deeper. Or if you're worried about the delay, it's possible to disable --depth= in Travis CI so that it'd clone the whole thing.

black-out · 2019-03-17T22:54:40Z

🐍🌚🤖 @Mariatta, I've formatted these files using black:

cherry_picker/cherry_picker/test.py
(I'm a bot 🤖)

Mariatta · 2019-03-17T22:57:05Z

Thanks, I've disabled git depth in travis CI and reverted the mocks.

webknjaz · 2019-03-17T22:59:55Z

@Mariatta great!

P.S. One small feedback about black out: as you can see it messes up unrelated stuff making it impossible to track down (by looking at diff) what was the intended change in the PR. It's not completely non-atomic. I'd recommend applying black to the project separately and then when PR is on top of that, using back will only generate related changes :)

Mariatta · 2019-03-17T23:03:50Z

Thanks!

the-knights-who-say-ni added the CLA signed label Mar 15, 2019

Mariatta changed the title ~~Cherry-picker: make BACKPORT_COMPLETE an allowed state~~Cherry-picker: Remove BACKPORT_COMPLETE. UNSET state after finished.Mar 15, 2019

webknjaz approved these changes Mar 15, 2019
View reviewed changes

Mariatta added the black out Runs black on PR label Mar 15, 2019

black-outBot removed the black out Runs black on PR label Mar 15, 2019

Mariattaand others added 4 commits March 16, 2019 12:46

Cherry-picker: make BACKPORT_COMPLETE an allowed state
ad1e54a

Remove backport_complete, and unset instead.
9cb6885

🐍🌚🤖 Formatted using black.
3f29a0e

🐍🌚🤖 Formatted using black.
e6e6c97

Mariatta force-pushed the fix-state branch from f7f574f to e6e6c97Compare March 16, 2019 19:50

Mariatta added the black out Runs black on PR label Mar 16, 2019

🐍🌚🤖 Formatted using black.
1c816d0

black-outBot removed the black out Runs black on PR label Mar 16, 2019

mock the validate_sha
36f0732

webknjaz reviewed Mar 17, 2019
View reviewed changes

Comment threadcherry_picker/cherry_picker/cherry_picker.py Outdated

webknjaz reviewed Mar 17, 2019
View reviewed changes

mock the validate_sha
8acc8a1

Mariatta force-pushed the fix-state branch from 377d27c to 8acc8a1Compare March 17, 2019 22:25

webknjaz requested changes Mar 17, 2019
View reviewed changes

Mariatta added the black out Runs black on PR label Mar 17, 2019

🐍🌚🤖 Formatted using black.
b90ac79

black-outBot removed the black out Runs black on PR label Mar 17, 2019

Disable git depth in travis
a97a4c8

Mariatta added the black out Runs black on PR label Mar 17, 2019

🐍🌚🤖 Formatted using black.
24d5394

black-outBot removed the black out Runs black on PR label Mar 17, 2019

Mariatta merged commit 04b6587 into masterMar 17, 2019

Mariatta deleted the fix-state branch March 17, 2019 23:03

webknjaz mentioned this pull request Mar 29, 2019
cherry_picker 1.3.1 breaks miss islington #320
Closed

Mariatta added the cherry_picker.py label May 27, 2019

Uh oh!

Uh oh!

Conversation

Mariatta commented Mar 15, 2019

Uh oh!

Mariatta commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

Mariatta commented Mar 15, 2019

Uh oh!

Mariatta commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

Mariatta commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

Mariatta commented Mar 15, 2019

Uh oh!

black-outBot commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

webknjaz commented Mar 15, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webknjaz commented Mar 15, 2019

Uh oh!

black-outBot commented Mar 16, 2019

Uh oh!

Uh oh!

webknjazMar 17, 2019

Choose a reason for hiding this comment

Uh oh!

MariattaMar 17, 2019

Choose a reason for hiding this comment

Uh oh!

webknjazMar 17, 2019

Choose a reason for hiding this comment

Uh oh!

webknjaz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Mar 17, 2019

Uh oh!

black-outBot commented Mar 17, 2019

Uh oh!

webknjaz commented Mar 17, 2019

Uh oh!

black-outBot commented Mar 17, 2019

Uh oh!

Mariatta commented Mar 17, 2019

Uh oh!

webknjaz commented Mar 17, 2019

Uh oh!

Mariatta commented Mar 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

webknjaz commented Mar 15, 2019•
edited
Loading

webknjaz left a comment •
edited
Loading