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

Fixed CommandLineUtilsTest for german locale #4219

Merged
merged 4 commits into from Jun 29, 2023

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented May 5, 2023

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Test Fix

Description

CommandLineUtil was failing for german locales because we translate text we're asserting on into german.

Fixes #4216

Things to be aware of

Just fixes the test

Things to worry about

Nothing

@mkarg
Copy link
Contributor

mkarg commented May 6, 2023

I don't think it is a good idea to simply turn "Version" into "version". It might work with German locale, but what about language having a completely different word for "Version" (like Polish "wersja")?

@mkarg
Copy link
Contributor

mkarg commented May 6, 2023

I don't think it is a good idea to let the test use the code that is to be tested. What shall be the use of this? If there is a typo in the bundle file, the test shall guarantee that the typo still exists?!

@nvoxland
Copy link
Contributor Author

nvoxland commented May 9, 2023

I think it's fine to be using the coreBundle.getString in this test. The goal of the test is to ensure the banner is showing/not showing and it's not trying to test the actual wording. The original failure was because we were just checking the wording as an easy assertion, but that wording can change in german. So just checking "is the expected translated text shown (or not)" is still testing the wanted "is the header shown?" check.

The Version/version check is best done with the coreBundle too, good catch. I had gotten lazy apparently. I don't think we'll have any other translations beyond german (that may go away at some point even too) but still best to be more correct in the test.

I ran all the tests locally in german (I think) and didn't get any other failure. Let me know if you're seeing any more.

Copy link
Contributor

@mkarg mkarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give me some days. I will check on German laptop next week. Thanks!

@mkarg
Copy link
Contributor

mkarg commented May 24, 2023

Unfortunately I'm busy with our own software currently, but stay tuned, a full test on German locale will follow ASAP. Good that Paul triggered me today, so I had a reminder. :-)

@mkarg
Copy link
Contributor

mkarg commented Jun 2, 2023

Sorry for the delay. Here is the test result on a German Windows 10 Pro:

[ERROR] Failures:
[ERROR]   CSVReaderTest.invalid csv:41 Condition not satisfied:

e.message.startsWith("Unterminated quoted field at end of CSV line")
| |       |
| |       false
| Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ['' , '' ,
| ]
com.opencsv.exceptions.CsvMalformedLineException: Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ['' , '' ,
]
        at com.opencsv.CSVReader.primeNextRecord(CSVReader.java:245)
        at com.opencsv.CSVReader.flexibleRead(CSVReader.java:610)
        at com.opencsv.CSVReader.readNext(CSVReader.java:204)
        at liquibase.util.csv.CSVReader.readNext(CSVReader.java:36)
        at liquibase.util.csv.CSVReaderTest.$spock_feature_0_1(CSVReaderTest.groovy:37)

[ERROR]   CSVReaderTest.invalid csv:41 Condition not satisfied:

e.message.startsWith("Unterminated quoted field at end of CSV line")
| |       |
| |       false
| Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ['g' , 'h' ,
| ]
com.opencsv.exceptions.CsvMalformedLineException: Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ['g' , 'h' ,
]
        at com.opencsv.CSVReader.primeNextRecord(CSVReader.java:245)
        at com.opencsv.CSVReader.flexibleRead(CSVReader.java:610)
        at com.opencsv.CSVReader.readNext(CSVReader.java:204)
        at liquibase.util.csv.CSVReader.readNext(CSVReader.java:36)
        at liquibase.util.csv.CSVReaderTest.$spock_feature_0_1(CSVReaderTest.groovy:37)

[ERROR]   CSVReaderTest.invalid csv:41 Condition not satisfied:

e.message.startsWith("Unterminated quoted field at end of CSV line")
| |       |
| |       false
| Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ["def
| ]
com.opencsv.exceptions.CsvMalformedLineException: Unterminiertes Anf├╝hrungszeichen am Ende einer CSV-Zeile. Anfang des verlorenen Textes: ["def
]
        at com.opencsv.CSVReader.primeNextRecord(CSVReader.java:245)
        at com.opencsv.CSVReader.flexibleRead(CSVReader.java:610)
        at com.opencsv.CSVReader.readNext(CSVReader.java:204)
        at liquibase.util.csv.CSVReader.readNext(CSVReader.java:36)
        at liquibase.util.csv.CSVReaderTest.$spock_feature_0_1(CSVReaderTest.groovy:37)

[INFO]
[ERROR] Tests run: 3825, Failures: 3, Errors: 0, Skipped: 7

Unfortunately there seems to be more code that relies on particular behavior. Do you want to add that fix, or do you want me to provide one? This issue is a showstopper for me, as I cannot test my contribution regarding SQL Anywhere TIMESTAMP.

NB: I think it is not a good idea that any test is dependend of the locale in any way and would highly recommend removing any such dependencies instead of fixing them.

@filipelautert
Copy link
Collaborator

Hello @mkarg - I faced the same problem on Windows pt-br:

Condition not satisfied:

e.message.startsWith("Unterminated quoted field at end of CSV line")
| |       |
| |       false
| Delimitador de fim de campo texto não encontrado ao final da linha CSV. Começo do texto perdido: ('' , '' ,
| ).

I believe that CSVReaderTest is the last one that needs to be fixed. I changed the test to use the line number instead of verifying the message.

Could you check if the build works for you now?

@mkarg
Copy link
Contributor

mkarg commented Jun 20, 2023

Guys, here is bad news and good news! :-)

Thank you, @filipelautert, for the latest fix!

☹️ Bad news: Unfortunately still the test suite fails to succeed on a German Windows 10 Pro. This time it hangs here:

[ERROR] Tests run: 348, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 18.838 s <<< FAILURE! - in liquibase.integrationtest.LiquibaseCoreExtensionTests
[ERROR] liquibase.extension.testing.command.CommandTests.Run {db:h2,command:generateChangelog} File already exists and no overwrite parameter provided  Time elapsed: 0.034 s  <<< FAILURE!
org.junit.ComparisonFailure: Exception message does not contain expected expected:<[Output ChangeLogFile 'target/test-classes/changelog-test.xml' already exis]ts!> but was:<[Ausgabe-ChangeLogFile 'target/test-classes/changelog-test.xml' existiert berei]ts!>
        at liquibase.extension.testing.command.CommandTests.checkOutput(CommandTests.groovy:622)
        at liquibase.extension.testing.command.CommandTests.run_closure8(CommandTests.groovy:347)
        at liquibase.extension.testing.command.CommandTests.run_closure8(CommandTests.groovy)
        at groovy.lang.Closure.call(Closure.java:418)
        at liquibase.Scope.child(Scope.java:203)
        at liquibase.Scope.child(Scope.java:179)
        at liquibase.extension.testing.command.CommandTests.run(CommandTests.groovy:326)

😃 Good news: @nvoxland Please cherry-pick my commit from mkarg@87ca520 into PR #4219, as it is the last missing brick to successfully pass tests on German Windows 10 Pro! 🚀

@filipelautert
Copy link
Collaborator

Thanks @mkarg ! As Nathan is in PTO I cherry-picked your commit.

Copy link
Collaborator

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR Enables liquibase tests to run regardless of the machine locale.

@filipelautert filipelautert merged commit fdda73a into master Jun 29, 2023
27 checks passed
@filipelautert filipelautert deleted the fix-commandlineutil-test branch June 29, 2023 19:23
@filipelautert filipelautert added this to the 1NEXT milestone Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Test fails on non-English locales
6 participants