Skip to content

Conversation

stonio
Copy link
Contributor

@stonio stonio commented Jun 28, 2016

Ensure Maven tests are OK for any language/file encoding.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 1af1a09 on stonio:patch-9 into 99312e3 on bramp:master.

stonio added 3 commits June 28, 2016 16:09
Test "testGetMediaInformation()" fails in environment where default locale has a comma as decimal mark: German, French...
@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling e3f35b3 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling ca842d6 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling ca842d6 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling ca842d6 on stonio:patch-9 into 99312e3 on bramp:master.

FFmpegFormat format = probeResult.getFormat();
String line1 =
String.format("File: '%s' ; Format: '%s' ; Duration: %.3fs", format.filename,
String.format(Locale.US, "File: '%s' ; Format: '%s' ; Duration: %.3fs", format.filename,
Copy link
Owner

Choose a reason for hiding this comment

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

Can we just call Locale.setDefault before the test, to avoid cluttering up the example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Method Locale#setDefault(Locale) changes locale for the instance of the JVM. I would rather use a specific locale only for this test ; maybe via a local field ?

@stonio
Copy link
Contributor Author

stonio commented Jun 28, 2016

We should also ensure tests are OK whatever the OS language or file encoding. I have not found the way to configure these in Travis-CI.

@bramp
Copy link
Owner

bramp commented Jun 28, 2016

I agree tests should work regardless of the Locale. However, in this case we are depending on the specific formatting, and I want to keep the documentation simple.

I would only want Locale.setDefault for this one test. Can we now Locale.setDefault in a setup, and undo it in a teardown method?

Use method Locale#setDefault(Locale) to force default local temporarily.
@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 37cf6b5 on stonio:patch-9 into 99312e3 on bramp:master.

@stonio
Copy link
Contributor Author

stonio commented Jun 28, 2016

I have added some cumbersome code to use Locale#setDefault(Locale).

However, this code is not thread-safe and can have side-effects with Parallel Test Execution.

@coveralls
Copy link

coveralls commented Jun 28, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 18dbceb on stonio:patch-9 into 99312e3 on bramp:master.

@bramp
Copy link
Owner

bramp commented Jun 29, 2016

You make a good case that this code is no longer thread-safe. So perhaps we annotate with @NotThreadSafe, or perhaps we just change the test to use format(locale,...), and keep the README the simple.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 9a6a039 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 9a6a039 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 1d72217 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling faae064 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 5655ef4 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling 7958202 on stonio:patch-9 into 99312e3 on bramp:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 83.65% when pulling f702ba0 on stonio:patch-9 into 99312e3 on bramp:master.

@bramp bramp merged commit 69d8569 into bramp:master Jul 24, 2016
@stonio stonio deleted the patch-9 branch July 25, 2016 08:15
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.

3 participants