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

Skip locale-dependent tests if the required locale is not installed #605

Merged

Conversation

Gallaecio
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #605 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #605   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files         302      302           
  Lines        2503     2503           
=======================================
  Hits         2381     2381           
  Misses        122      122

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b493f...37f379d. Read the comment docs.

Copy link
Collaborator

@noviluni noviluni left a comment

Choose a reason for hiding this comment

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

That's really cool!

However, I'm not sure if we can merge it as is, because if for some reason (by error) we skip some locales in the pipeline, we would skip them without knowing it.

What do you think @Gallaecio ? Do you know if is there any way to detect that the tests are running within a pipeline and raise an error in that case if a locale is missing?

@Gallaecio
Copy link
Member Author

I see your point, but to skip the required French locale in Travis CI we would have to remove line https://github.com/scrapinghub/dateparser/blob/master/.travis.yml#L30 so I’m not really concerned about such an scenario.

Copy link
Collaborator

@noviluni noviluni left a comment

Choose a reason for hiding this comment

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

Ok, I see it clearer now :)

@asadurski
Copy link
Member

Good one!
We should probably add similar test for locale that's further away from English (not as part of this PR though).

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