gojuno / composer

Reactive Android Instrumentation Test Runner. Archived. Marathon is recommended as an alternative (https://github.com/Malinskiy/marathon).
Apache License 2.0
546 stars 45 forks source link

Make linefeed OS-agnostic in unit tests #118

Closed Nilzor closed 6 years ago

Nilzor commented 6 years ago

Ensure no tests fail due to \n differs to \r\n on Windows, while maintaining Linux/MacOS compatibility

edit: Might hold off the merge until I've verified that tests in HtmlReportSpec passes. I'm struggling to generate app.min.js (or you can verify it on AppVeyor)

edit 2: HtmlReportSpec have other issues not related to Linefeed or Windows. I believe this PR can be merged

Nilzor commented 6 years ago

Appveyor result: https://ci.appveyor.com/project/Nilzor/composer/build/appveyor%204#L454

Down to 6 errors - all related to HtmlReportSpec.kt, which requires a lot more work to fix on Windows (Not linefeed-related)

yunikkk commented 6 years ago

@Nilzor regarding 6 issues with the HtmlReportSpec.kt - tests expect Nodejs is present and are supposed to work in Docker container with ci/build.sh, which contains all the needed environment for that. I guess we'll need a separate script and container for running all on Windows (or is running Linux Docker images possible/not painful on Windows, I'm not sure really).

Nilzor commented 6 years ago

Do we really need to be able to run the tests on Windows though? Having the production code running on Windows and having the development environment run are two different things.

Running docker on Windows is definitely possible, I'm just not sure it's worth investing in it. There are a couple of approaches we could take:

I'm leaning towards number 3, or not doing anything at all with regards to testing on Windows

Nilzor commented 6 years ago

PS: Thanks for the merge - could you also please make a new release build?

yunikkk commented 6 years ago

Well, running all tests on Windows would make us sure that all stuff continues working on Windows after new changes. Let's stick with option 3 for now, ideally considering 1 one somewhere in future)

yunikkk commented 6 years ago

Pushed new release, too.