Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Cherry-picker: Remove BACKPORT_COMPLETE. UNSET state after finished.#315

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Mariatta merged 10 commits intomasterfromfix-state
Mar 17, 2019

Conversation

Mariatta
Copy link
Member

No description provided.

@Mariatta
Copy link
MemberAuthor

@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 reacted with eyes emoji

@webknjaz
Copy link
Contributor

Checking it now.

@webknjaz
Copy link
Contributor

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

@webknjaz
Copy link
Contributor

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

@MariattaMariatta changed the titleCherry-picker: make BACKPORT_COMPLETE an allowed stateCherry-picker: Remove BACKPORT_COMPLETE. UNSET state after finished.Mar 15, 2019
@Mariatta
Copy link
MemberAuthor

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

webknjaz reacted with rocket emoji

@Mariatta
Copy link
MemberAuthor

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

@webknjaz
Copy link
Contributor

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

@Mariatta
Copy link
MemberAuthor

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

@webknjaz
Copy link
Contributor

I checked this branch locally as well and it passes.

@webknjaz
Copy link
Contributor

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

@Mariatta
Copy link
MemberAuthor

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

@MariattaMariatta added the black outRuns black on PR labelMar 15, 2019
@black-out
Copy link

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

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

@black-outblack-outbot removed the black outRuns black on PR labelMar 15, 2019
@webknjaz
Copy link
Contributor

@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
Copy link
Contributor

webknjaz commentedMar 15, 2019
edited
Loading

@Mariatta I know what the problem is! 🎉

Sogit 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
Copy link
Contributor

Solutions:

  1. Change the sha in config to something newer
  2. 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
Copy link

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

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

@black-outblack-outbot removed the black outRuns black on PR labelMar 16, 2019
],
)
@mock.patch("os.path.exists")
@mock.patch("cherry_picker.cherry_picker.validate_sha")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@webknjazwebknjaz left a comment
edited
Loading

Choose a reason for hiding this comment

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

I think that mockingvalidate_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
Copy link
MemberAuthor

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

@MariattaMariatta added the black outRuns black on PR labelMar 17, 2019
@black-out
Copy link

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

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

@black-outblack-outbot removed the black outRuns black on PR labelMar 17, 2019
@webknjaz
Copy link
Contributor

@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.

@MariattaMariatta added the black outRuns black on PR labelMar 17, 2019
@black-out
Copy link

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

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

@black-outblack-outbot removed the black outRuns black on PR labelMar 17, 2019
@Mariatta
Copy link
MemberAuthor

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

webknjaz reacted with hooray emoji

@webknjaz
Copy link
Contributor

@Mariatta great!

P.S. One small feedback aboutblack 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 reacted with thumbs up emoji

@MariattaMariatta merged commit04b6587 intomasterMar 17, 2019
@Mariatta
Copy link
MemberAuthor

Thanks!

webknjaz reacted with eyes emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@webknjazwebknjazwebknjaz requested changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@Mariatta@webknjaz@the-knights-who-say-ni

[8]ページ先頭

©2009-2025 Movatter.jp