Added CheckLibraryVersion class for check library version #142
Conversation
|
Please, don't add unnecessary dependencies to the |
| @@ -138,6 +139,8 @@ private void printTestNum(int num) { | |||
|
|
|||
| @Test | |||
| public final void start() { | |||
| CheckLibraryVersion checkLibraryVersion = new CheckLibraryVersion(); | |||
| checkLibraryVersion.checkVersion(); | |||
There was a problem hiding this comment.
Every executable code of hs-test should be under try-catch block. Including this one. It guarantees that in case something goes wrong, the user would still get some message from testing (yes, it would be Unexpected error but it is better than just crash with exception)
There was a problem hiding this comment.
Also, why do we even need to check the library beforehand if we use this information only in case the user failed test? We should check the version only in case of fail as well.
There was a problem hiding this comment.
We have two variants to check,
if (!currentVersion.equals(latestVersion)) { if (currentVersion.split("\\.")[0] != latestVersion.split("\\.")[0]) { throw new RuntimeException(getFeedback()); } else { isLatestVersion = false; } }
if (currentVersion.split("\\.")[0] != latestVersion.split("\\.")[0]) { throw new RuntimeException(getFeedback());
if the user's version and Git do not equal in main index X.0.0 user has RuntimeException(getFeedback()); and process stoped,
and light checking when the user has failed on the test case.
There was a problem hiding this comment.
What kind of feature should it be for the release of (X+1).0.0 so that every user needs to update to it right now? Every major release added support for major technology or language. For example: support of Go testing, support of Bash testing, support of Spring Boot testing. In case the users already testing their applicarion chances are that they simply don't need new major language or technology supported in hs-test. Because whatever they are testing already supported in hs-test.
|
|
||
| int responseCode = connection.getResponseCode(); | ||
| if (responseCode != 200) { | ||
| throw new UnexpectedError("Error getting latest version of hs-test library from GitHub"); |
There was a problem hiding this comment.
Ok, in case GitHub is down our users can't even test their code. It is not OK. I think, in case we can't check library version, you should assume it's the latest version.
There was a problem hiding this comment.
Also, keep in mind that online checking (from the website) is done in the isolated environment and there are no internet connection. Such way of checking should continue working fine. We should not rely on internet being accessible while checking user code. Could you please tell what is a motivation behind this issue?
There was a problem hiding this comment.
I think you mean that with online verification it will not be possible to get the current version in GitHub, the latestVersion will be equal to the currentVersion and the user will continue to work with tests.
The relevance of the library to work in images for online verification is not the task of the user, we must maintain the relevance of the library for online verification.
There was a problem hiding this comment.
@polischuks Yes. But online checking in done without access to the internet (Docker image has no access to he internet) and that is why the check will fail. That's what I mean
|
@aaaaaa2493
|
|
@polischuks please remove the following from the dependencies |
| lastChecked = LocalDate.parse(reader.readLine()); | ||
| reader.close(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Why this method contains a lot of RuntimeException's? That's not a good way of handling checked exceptions making them unchecked exceptions. At the very least they should be UnexpectedError's. Or just make the method as thorows IOException
|
@aaaaaa2493 fixed |
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| throw new IOException(e); |
There was a problem hiding this comment.
It's basically the same as in throwing RuntimeException. You can just omit catch block, it's useless
| @@ -68,7 +61,7 @@ public void checkVersion() { | |||
| try (FileWriter writer = new FileWriter(lastCheckedFile)) { | |||
| writer.write(lastChecked.toString()); | |||
| } catch (IOException e) { | |||
| throw new RuntimeException(e); | |||
| throw new IOException(e); | |||
|
@aaaaaa2493 fixed/ removed catch blocks |
| } | ||
| } | ||
|
|
||
| if (lastChecked != null && lastChecked.equals(LocalDate.now())) { |
There was a problem hiding this comment.
| if (lastChecked != null && lastChecked.equals(LocalDate.now())) { | |
| if (LocalDate.now().equals(lastChecked)) { |
| private void getLatestHsTestVersionFromGitHub() { | ||
| HttpURLConnection connection = null; | ||
| try { | ||
| URL url = new URL("https://api.github.com/repos/hyperskill/hs-test/releases/latest"); |
| */ | ||
| public void checkVersion() throws IOException { | ||
| LocalDate lastChecked = null; | ||
| File lastCheckedFile = new File("LastChecked.txt"); |
There was a problem hiding this comment.
Check what this file doesn't put into submission.
Task: #HSPC-70
Dependencies
Reviewers
Description