Closed phalestrivir closed 6 months ago
vscheuber
commented
6 months ago @phalestrivir would you mind rebasing and updating your PR? The PR looks generally great but I noticed 43 failing tests, all client_cli/en/* tests with minor help text updates I made over the past little while and I am wondering what other changes might have not made it into your fork? Just a quick sanity check would help.
phalestrivir
commented
6 months ago @phalestrivir would you mind rebasing and updating your PR? The PR looks generally great but I noticed 43 failing tests, all
client_cli/en/*tests with minor help text updates I made over the past little while and I am wondering what other changes might have not made it into your fork? Just a quick sanity check would help.
I tried rebasing but it says the PR is up to date with the Rockcarver main branch (which is what we branched off of), so I'm not sure what changes you are referring to that might not have made it in, unless they are in some other branch besides main. I re-ran all the client_cli/en/* tests on my end just to make sure and they are all passing for me.
vscheuber
commented
6 months ago Ok, thank you for checking! Maybe there is something wrong in my side… I’ll check again when I get to my hotel.Sent from my iPhoneVolker Scheuber801.473.5451On May 7, 2024, at 2:07 PM, phalestrivir @.***> wrote:
@phalestrivir would you mind rebasing and updating your PR? The PR looks generally great but I noticed 43 failing tests, all client_cli/en/* tests with minor help text updates I made over the past little while and I am wondering what other changes might have not made it into your fork? Just a quick sanity check would help.
I tried rebasing but it says the PR is up to date with the Rockcarver main branch (which is what we branched off of), so I'm not sure what changes you are referring to that might not have made it in, unless they are in some other branch besides main. I re-ran all the client_cli/en/* tests on my end just to make sure and they are all passing for me.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>
phalestrivir
commented
6 months ago I noticed the Actions are failing for this PR. I looked at when the frodo-lib changes were added and they were added after the actions ran for this PR, so if you run the Actions now they should all pass.
This PR updates the help command by adding a custom Help object for FrodoStubCommands. Additionally, it refactors all sub-commands to run in the same process instead of running the sub-commands as executables in separate processes. Thus, everything should be functionally the same besides this change to how the commands are run.
Due to the changes to how the commands are created, there can no longer be two descriptions for each sub-command, but just one description. This isn't a huge deal since a lot of the descriptions were the same or very similar anyways, but it did mean that we had to update the client_cli snapshots where the descriptions differed. Similarly, due to the changes in the help command, the client_cli snapshots had to be updated.
No changes needed to be made to e2e tests or snapshots however. They were originally failing after the changes due to the mock naming scheme no longer working as it was before, likely due to the fact that it is now using only one process to run all sub-commands, resulting in the mocks generating differently now. We made a temporary fix in frodo-lib to how mocks are named to make it so the tests work with the current mocks, which fix is in another PR on the frodo-lib project, although the mock naming is something we will want to improve eventually.