Styx tests should pass in windows #101
Conversation
There was a problem hiding this comment.
I have left some comments.
I think we need to discuss how best to make Styx code multi-platform together before we try to make the changes.
On an unrelated, but important note, this branch fails to build on my machine, with checkstyle violations. We should also determine how best to run a windows-equivalent of make e2e (which is how we build locally on Mac).
| * Copyright (C) 2013-2017 Expedia Inc. | ||
| * | ||
| * Copyright (C) 2013-2018 Expedia Inc. | ||
| * <p> |
There was a problem hiding this comment.
We need to remove any formatting "help" that our IDEs add to the copyright notice, because it will cause the build process to add another copyright notice on top of it.
There was a problem hiding this comment.
I have addressed these issues with the IDE in another comment. Other than that, do you have any document with coding guidelines you follow? For instance, for the "use string literals" in tests part.
| newPath = path.getParent(); | ||
| private String removeAsterisk(String path) { | ||
| String newPath = path; | ||
| if (path.endsWith("*")) { |
There was a problem hiding this comment.
This logic is not the same. The old code checks that the final part of the path is *, e.g. foo/bar/*, which is more strict than simply checking if the final character is *.
Do you believe the old logic is incorrect?
There was a problem hiding this comment.
So can the path have "*" anywhere other than after a slash? There are no tests for that condition in PathTrieTest. In Windows * is not a valid character for filenames, so I understood it was safe to remove it in any case. If it is not, I think we might want to make code more explicit regarding what Paths are valid.
|
|
||
| @Test | ||
| public void listsResourcesFromFileSystemDirectory() { | ||
| Iterable<Resource> jars = resourceIndex.list(PLUGINS_FIXTURE_PATH.toString(), ".jar"); | ||
|
|
||
| assertThat(jars, contains(resourceWithPath("oneplugin/url-rewrite-1.0-SNAPSHOT.jar"))); | ||
| assertThat(jars, contains(resourceWithPath(EXISTING_PLUGIN_RELPATH))); |
There was a problem hiding this comment.
We follow a principle of leaving literal strings in tests where possible so that readers do not need to jump around the file to understand the test.
| @@ -26,7 +26,7 @@ | |||
| public void canAcquireClasspathResources() { | |||
| Resource resource = ResourceFactory.newResource("classpath:com/hotels/styx/api/io/resource.txt"); | |||
|
|
|||
| assertThat(resource, contains("This is an example resource.\nIt has content to use in automated tests.")); | |||
| assertThat(resource, contains("This is an example resource."+System.lineSeparator()+"It has content to use in automated tests.")); | |||
There was a problem hiding this comment.
We may want to consider using String.format (with static imports) for these line separator cases.
e.g. assertThat(resource, contains(format("This is an example resource.%nIt has content to use in automated tests.")));
| import static com.hotels.styx.StartupConfig.LOGBACK_CONFIG_LOCATION_VAR_NAME; | ||
| import static com.hotels.styx.StartupConfig.STYX_HOME_VAR_NAME; | ||
| import static com.hotels.styx.StartupConfig.newStartupConfigBuilder; | ||
| import static com.hotels.styx.StartupConfig.*; |
There was a problem hiding this comment.
We don't use the * import in Styx as we like to only import exactly what we are using.
There was a problem hiding this comment.
As discussed, that's due to IntelliJ settings. The IDE itself was changing the imports as per the "count to use (static) import with *" If you could do an export of the Settings -> Editor -> Code Style to verify that I don't have any other settings configured that break codestyle's expectations, that would be great.
| @@ -107,7 +107,8 @@ public void createsMetricsJsonCorrectly() throws JsonProcessingException { | |||
| "\n" + | |||
| " },\n" + | |||
| " \"version\":\"3.1.3\"\n" + | |||
| "}")); | |||
| "}"; | |||
| assertThat(after, is(expectedString.replace("\n", System.lineSeparator()))); | |||
There was a problem hiding this comment.
We should try to find a consistent way to handle the line-separator cross-platform issue, so that we are not cluttering the code too much.
dvlato
force-pushed
the
styx-tests-should-pass-in-windows
branch
from
5d3f05d
to
fe49cd1
Compare
…nce improvements.
…les are still failing.
…ins file is open so we cannot copy it)
…ll need to fix the issue with FileBackedRegistry.readFile() not closing the inputstream.
…ll need to fix the issue with FileBackedRegistry.readFile() not closing the inputstream.
…nce improvements.
dvlato
force-pushed
the
styx-tests-should-pass-in-windows
branch
from
520850d
to
b75371d
Compare
Fixed several issues when trying to run the build in Windows.