-
Notifications
You must be signed in to change notification settings - Fork 26
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
LUAFDN-784 Try to download all tools before erroring #62
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I have read the CLA Document and I hereby sign the CLA |
tests/cli.rs
Outdated
stylua = { github = "JohnnyMorganz/StyLua", version = "0.11.3" } | ||
not-a-real-tool = { github = "Roblox/VeryFakeRepository", version = "0.1.0" } | ||
badly-formatted-tool = { github = "Roblox/", version = "0.2.0" } | ||
selene = { source = "Kampfkarren/selene", version = "=0.22.0" } |
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.
This test ends up actually downloading these tools. Is there any way to mock downloading stylua and selene? I want to have a "successful" download alongside the failed ones.
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.
This is possible yes but you will have to refactor the code to inject a custom ToolProvider implementation. This is probably easier to do within the src
code. Tests in tests
are considered at least integration level and so they cannot access private members. So if you refactor the code a little, you can write a unit test in main.rs
that injects a memory only tool provider
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'll just add an e2e test with the actual downloads and just test out the download failure here. I don't want the scope of this PR to get too large, so I might just file an issue to inject a mocked tool provider if there isn't one already.
…-37-process-all-tools
Problem
Closes #37
LUAFDN-784 We should try to download all tools before erroring. Currently, the tool will immediately crash if it finds a tool that it cannot install. This is not great, as a foreman.toml file may list some tools that the user doesn't have auth for, and the user will have to manually delete the entry for that tool.
Solution
Instead of halting execution when we encounter an error, catch the error and add it to an error map. Then we can display all the errors accumulated during execution after all downloads have been attempted.
Testing
Added a snapshot test for downloading all of the tools, even if there's an error in the middle.