Skip to content
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

[sleepiq] Fix the failing tests on Windows #7983

Merged
merged 2 commits into from
Jun 23, 2020
Merged

Conversation

lolodomo
Copy link
Contributor

Fix #7981

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo requested a review from syphr42 as a code owner June 22, 2020 17:41
@lolodomo
Copy link
Contributor Author

Please read my comment in the issue, I am not sure that it is the best way to fix the problem with endline encoding.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@wborn
Copy link
Member

wborn commented Jun 23, 2020

I just copy pasted the files from the orignal repo. On my machines Git is configured to not automatically convert line endings to my OS default. I think your Git is configured to automatically convert line endings? That's why you probably run into such issues.

Gson apparently always serializes JSON with \n line endings. What might be better is to use Files.readAllLines instead of Files.readAllBytes and then join the lines with \n . That way other line endings would also be supported, see:

    /**
     * Read all lines from a file. This method ensures that the file is
     * closed when all bytes have been read or an I/O error, or other runtime
     * exception, is thrown. Bytes from the file are decoded into characters
     * using the specified charset.
     *
     * <p> This method recognizes the following as line terminators:
     * <ul>
     *   <li> <code>&#92;u000D</code> followed by <code>&#92;u000A</code>,
     *     CARRIAGE RETURN followed by LINE FEED </li>
     *   <li> <code>&#92;u000A</code>, LINE FEED </li>
     *   <li> <code>&#92;u000D</code>, CARRIAGE RETURN </li>
     * </ul>
     * <p> Additional Unicode line terminators may be recognized in future
     * releases.
     *
     * <p> Note that this method is intended for simple cases where it is
     * convenient to read all lines in a single operation. It is not intended
     * for reading in large files.
     *
     * @param   path
     *          the path to the file
     * @param   cs
     *          the charset to use for decoding
     *
     * @return  the lines from the file as a {@code List}; whether the {@code
     *          List} is modifiable or not is implementation dependent and
     *          therefore not specified
     *
     * @throws  IOException
     *          if an I/O error occurs reading from the file or a malformed or
     *          unmappable byte sequence is read
     * @throws  SecurityException
     *          In the case of the default provider, and a security manager is
     *          installed, the {@link SecurityManager#checkRead(String) checkRead}
     *          method is invoked to check read access to the file.
     *
     * @see #newBufferedReader
     */
    public static List<String> readAllLines(Path path, Charset cs) throws IOException {

@lolodomo
Copy link
Contributor Author

lolodomo commented Jun 23, 2020

Ok, reading how Git is working, that is now clearer for me and I understand that all .json files are converted from LF to CRLF on Windows. It explains why I have CRLF on these files while in Git database they have only LF.
So the files themself are not the problem, the problem is clearly that the current tests are not compatible with Windows platform but only work on Linux. Your proposal consisting in changing the reading of the file in the tests looks as the best solution. I will go in this direction.

Note that I see that our .gitattributes file contains:

 java text=auto
.xml text=auto

Maybe we should add .json text=auto ? Of course, it will not solvce the current problem.

Signed-off-by: Laurent Garnier <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @lolodomo,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@lolodomo
Copy link
Contributor Author

Done. Ready for a review & merge.

@lolodomo lolodomo changed the title [sleepiq] Fix the failing tests [sleepiq] Fix the failing tests on Windows Jun 23, 2020
@wborn
Copy link
Member

wborn commented Jun 23, 2020

Maybe we should add .json text=auto ?

That makes sense since .xml is already there. It will make checkouts between users of the same OS more consistent. It might initially cause some test failures. Linux is tested using Travis/Jenkins so it will be easy to see if tests still work. You've already checked conversion to Windows line endings yourself. 😉

@lolodomo
Copy link
Contributor Author

lolodomo commented Jun 23, 2020

The change of .gitattributes is out of the scope of this PR. I have not tested it yet. In my case, with default Git settings, as nothing is set for .json files in .gitattributes, I understand that Git setting core.autocrlf is used which is set to true, meaning files will be converted to CRLF on Windows (core.eol is unset). Changing the file .gitattributes should have no impact in practice. It would just avoid involving different Git settings for .xml and .json files.

@wborn
Copy link
Member

wborn commented Jun 23, 2020

Yes that's how it works. Another benefit would be that it's also easier to search (and replace) text spanning multiple lines in many files when they consistently use the OS default line endings.

@wborn wborn merged commit bcab859 into openhab:2.5.x Jun 23, 2020
@lolodomo lolodomo deleted the sleepiq_tests branch June 23, 2020 16:27
@cpmeister cpmeister added this to the 2.5.7 milestone Jun 23, 2020
knikhilwiz pushed a commit to knikhilwiz/openhab2-addons that referenced this pull request Jul 12, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Daan Meijer <[email protected]>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sleepiq] Tests failing
4 participants