-
Notifications
You must be signed in to change notification settings - Fork 893
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
Conversation
There was a problem hiding this 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.
@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. |
@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. |
Wonderful news, thank you @dmarcoux |
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 |
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. |
Hey @kinnison, could you have a look please? I just pushed changes and I would like to have your opinion on the changes. |
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. |
Regarding the new tests, I based them on the existing |
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. |
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
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 |
I've decided that right now the extraneous |
There was a problem hiding this 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.
Closes #2355