Fix #4988. Handle Maven Surefire locale and timezone#5061
Fix #4988. Handle Maven Surefire locale and timezone#5061kcondon merged 2 commits intoIQSS:developfrom
Conversation
… properties, so users around the globe all use the same and tests don't fail.
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
Overall, this looks great but we should probably reword a bit what we say about timezone in the dev guide.
|
|
||
| If you installed Maven instead of Netbeans, run ``mvn package``. | ||
|
|
||
| NOTE: Do you use a locale different than ``en_US.UTF-8`` and a timezone different to ``UTC`` on your development machine? |
There was a problem hiding this comment.
This is a little confusing to me because the core development team at @IQSS is not in the "UTC" timezone. We're in Boston (well, across the river in Cambridge) so it's Eastern Time Zone which I guess is either "EST" or "EDT" according to https://en.wikipedia.org/wiki/Eastern_Time_Zone . Maybe this could say something like "we set our tests to UTC" for testing?
| <configuration> | ||
| <!-- testsToExclude come from the profile--> | ||
| <excludedGroups>${testsToExclude}</excludedGroups> | ||
| <argLine>-Duser.timezone=${project.timezone} -Dfile.encoding=${project.build.sourceEncoding} -Duser.language=${project.language} -Duser.region=${project.region}</argLine> |
There was a problem hiding this comment.
Just a heads up that we pass some command line arguments in the .travis.yml file in the root of the repo. Tests are passing just find at https://travis-ci.org/IQSS/dataverse without any timezone, language, or region specified but I just thought I'd mention this.
There was a problem hiding this comment.
Yes of course this works... 😉
Travis CI by default uses locale en_US.UTF-8 and sets TZ to UTC according to some stuff on StackOverflow and Github issues.
There was a problem hiding this comment.
Ok, did you see my comment above on dev-environment.rst ?
There was a problem hiding this comment.
Yepp - was AFK for a break... 😉 Had some 🇨🇳 dinner.
|
Hi @pdurbin I just made some changes to the text. Maybe you can review again and merge or reply 😄 |
|
@poikilotherm hi! I don't do the testing or merging but this pull request looks good to me as of 8d3eb8e so I moved it to QA. Thanks! |
|
@pdurbin You're welcome! 😸 |
|
Overall looks good. I haven't checked, but it would seem to make sense to make sure that "haven't checked" meaning I haven't run it in this branch - so it's possible that no changes are necessary. |
|
About the test suite script: it just calls Maven... The Surefire argLine arguments added in the PR are used by the plug-in when starting the JVM to run the tests. This is necessary as system properties are read to late to apply the settings (see various Maven tutorials and Surefire bugtracker where I got this stuff from). Thus I don't see any necessity to adjust the scripts Maven arguments. |
|
I just got aware that I broke test coverage reporting with my commit, because the cmd line arguments from JaCoCo were ditched from the surefire-plugin. I will fix this in #5071, too. SORRY!!! 😿 |
…{argLine}' in surefire plugin argLine option.
|
Oh! Thanks for catching this. I know we're only at 16% code coverage but I like how https://coveralls.io/github/IQSS/dataverse?branch=develop show's that code coverage is increasing over time, and yes, we'd like this reporting to continue working:
|
…{argLine}' in surefire plugin argLine option.
…{argLine}' in surefire plugin argLine option.
…{argLine}' in surefire plugin argLine option.
pdurbin
added this to the
4.9.3 - Optional File PIDs, Initial Internationalization Work milestone
4 participants
Please merge 😃
Related Issues
Pull Request Checklist