-
Notifications
You must be signed in to change notification settings - Fork 24
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
#689: Added url and removed try-catch-block #698
#689: Added url and removed try-catch-block #698
Conversation
Added url to ide-projects/build and removed try-catch-block
Pull Request Test Coverage Report for Build 11932760876Details
💛 - Coveralls |
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.
@leonrohne27 thanks for your PR and fixing this. Great work: you removed the "hack" and all tests are still green. 👍
Before we merge, I would like to discuss how we handle such test urls
files.
Its seems you have copied the urls
file of Java to all the other tools and then the URL in there does not make any sense. So:
- should we either have a generic dummy URL in all these files?
- or should we have something reasonable there? And if so should that include both the tool and its version in the URL? Or is that all pointless since that URL is most likely never ever relevant in the test and will always be ignored.
I was quite lax in the beginning but after we ended up in a partial mess of quite inconsistent test-data that sooner or later broke our tests, I want to think a step further.
However, IMHO it is also fine to go for a simply dummy URL for all these files but lets discuss and agree in the team first.
Apart from that all is fine and ready for merge.
I have created issue #706 for the idea of serving the software as compressed archive via wiremock.
The values for I have created an example (see commit linked below) for such |
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.
LGTM.
cli/src/test/resources/ide-projects/dotnet/_ide/urls/dotnet/dotnet/6.0.419/urls.txt
Show resolved
Hide resolved
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.
@leonrohne27 thanks for changing all the URLs. 👍
For Java I changed .jar
to .tgz
extension.
Now ready for merge.
Fixes: #689
Added url to ide-projects/build and removed try-catch-block