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

Describe how to remove components when update fails #2384

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Describe how to remove components when update fails #2384

merged 3 commits into from
Nov 17, 2020

Conversation

dmarcoux
Copy link
Contributor

@dmarcoux dmarcoux commented Jun 22, 2020

Closes #2355

tests/dist.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

I apologise for taking so long to get back to you.

The approach is not bad. I'm worried that you had to add a whole bunch of "bonus" components to our channels rather than using an existing scenario. I'd prefer a smaller change to one or more scenarios to make your testing possible if you need changes at all. In addition, as I noted on the test you are finding hard to make work, your chosen approach of modifying channels in-test is awkward and painful, and not an example of good basic test writing (from wherever you found it) instead you should try and use an existing scenario where as the day changes, components come and go. This more accurately models the nightly changes which you're trying to improve the situation for anyway.

tests/dist.rs Outdated Show resolved Hide resolved
tests/dist.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
@kinnison kinnison added this to the 1.23.0 milestone Jul 6, 2020
@kinnison
Copy link
Contributor

kinnison commented Jul 6, 2020

@dmarcoux I've selected this issue (and thus PR) for the 1.23.0 milestone. Are you likely to be able to continue work on it any time soon? If not, no worries, just let us know here so that someone else can pick up the work and complete it.

@dmarcoux
Copy link
Contributor Author

dmarcoux commented Jul 7, 2020

@kinnison Sorry for the delay in responding to your previous comments. I will have time again at the end of the month after the 25th of July.

@kinnison
Copy link
Contributor

kinnison commented Jul 7, 2020

Wonderful news, thank you @dmarcoux

@dmarcoux
Copy link
Contributor Author

dmarcoux commented Jul 7, 2020

I might be able to find some time to work on this before the 25th, but I cannot guarantee. Anyway, I already pushed some changes to the tests unavailable_components_with_different_target and unavailable_components_with_same_target. Could you let me know what you think @kinnison?

@kinnison
Copy link
Contributor

kinnison commented Jul 7, 2020

I'll take a look, but don't worry about this until you can dedicate a solid chunk of time, just do what you need to do in the meantime and come back when you're ready. No rush.

@dmarcoux dmarcoux marked this pull request as ready for review July 26, 2020 14:09
@dmarcoux
Copy link
Contributor Author

Hey @kinnison, could you have a look please? I just pushed changes and I would like to have your opinion on the changes.

@kinnison
Copy link
Contributor

The tests you add still feel very complicated to me; but perhaps that's needed -- let's resovle the point about how we think the UX should be first though, then I'll reconsider if we can test this more efficiently.

@dmarcoux
Copy link
Contributor Author

Regarding the new tests, I based them on the existing unavailable_component test.

@dmarcoux
Copy link
Contributor Author

dmarcoux commented Sep 7, 2020

Sorry for coming to this after so many weeks, but I won't be able to finish this. I hope somebody else can pick this up.

@kinnison
Copy link
Contributor

I'm working on tidying the last bits of this since @dmarcoux did a good job of getting as far as they did.

I'll push an updated set of commits as soon as I'm finished tweaking things.

I'm currently trying to tweak things so that we don't emit --target ... if it's not needed (i.e. it's the default target)

@kinnison
Copy link
Contributor

I've decided that right now the extraneous --target is less important.

@kinnison kinnison self-requested a review November 17, 2020 11:00
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

I think I'm good with this as-is. Assuming CI goes green.

@kinnison kinnison merged commit 8e77e51 into rust-lang:master Nov 17, 2020
@dmarcoux dmarcoux deleted the address-issue-2355 branch November 17, 2020 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Describe how to remove components when update fails
2 participants