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.

Sign up for GitHub

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

Jump to bottom

Aviation provider: added more airlines. #765

Merged
merged 8 commits into from Apr 17, 2023
Merged

Aviation provider: added more airlines. #765

merged 8 commits into from Apr 17, 2023

Conversation

L-Evg
Copy link
Contributor

@L-Evg L-Evg commented Apr 14, 2023

Added more items under "IATA_airline", "ICAO_airline" and "airline" sections.

@bodiam
Copy link
Contributor

bodiam commented Apr 14, 2023

Seems the test is failing due to a regex mismatch.

kingthorin
kingthorin reviewed Apr 14, 2023
View reviewed changes
src/test/java/net/datafaker/providers/base/AviationTest.java Outdated
@@ -18,12 +18,12 @@ void flight_ICAO() {

@Test
void flight_IATA() {
assertThat(aviation.flight("IATA")).matches("[A-Z]{2}[0-9]+");
assertThat(aviation.flight("IATA")).matches("[A-Z|0-9]{2}[0-9]+");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the character class really include pipe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me rephrase, the square brackets shouldn’t include pipe. In either test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I am confused by you comment. I altered the regexp because a flight ID could be like 2B2345 or A97887. Previous data set had no such airlines codes (2B, A9 etc) so the regexp was ok. Now, since a IATA airline code could include a digit the original test may fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, I got your point. Fixed.

panilya
panilya approved these changes Apr 14, 2023
View reviewed changes
Copy link
Collaborator

@panilya panilya left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix. LGTM!

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023 •
edited

Codecov Report

Merging #765 (410bf77) into main (e64dfa8) will increase coverage by 0.05%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #765      +/-   ##
============================================
+ Coverage     92.77%   92.82%   +0.05%     
- Complexity     2604     2633      +29     
============================================
  Files           282      284       +2     
  Lines          5161     5202      +41     
  Branches        533      537       +4     
============================================
+ Hits           4788     4829      +41     
  Misses          245      245              
  Partials        128      128

see 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@L-Evg L-Evg requested a review from panilya April 14, 2023 11:20
@kingthorin
Copy link
Collaborator

The current failure seem unrelated:

11:19:52.586 [INFO] Running net.datafaker.idnumbers.EsMXIdNumberTest
11:19:54.988 [ERROR] Tests run: 351, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 4.427 s <<< FAILURE! - in net.datafaker.FakerTest
11:19:54.988 [ERROR] net.datafaker.FakerTest.shouldNotApplyCachingToMethodsWithParameters  Time elapsed: 0.005 s  <<< FAILURE!
java.lang.AssertionError: 

Expecting actual:
  "8V7218"
to match pattern:
  "[A-z]{2}\d{1,4}"
	at net.datafaker.FakerTest.shouldNotApplyCachingToMethodsWithParameters(FakerTest.java:358

I’ll kick the tests off again.

@L-Evg
Copy link
Contributor Author

L-Evg commented Apr 14, 2023

@kingthorin - seems it fails because of a test for #716 The test uses the aviation provider's data and the "old" regexp. Let me fix that.

kingthorin
kingthorin approved these changes Apr 14, 2023
View reviewed changes
Copy link
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

Thanks

L-Evg and others added 2 commits April 17, 2023 09:07
@L-Evg @snuyanzin
Update src/test/java/net/datafaker/providers/base/AviationTest.java
f638003 
Co-authored-by: Sergey Nuyanzin <sergey.nuyanzin@aiven.io>
@L-Evg @snuyanzin
Update src/test/java/net/datafaker/providers/base/AviationTest.java
410bf77 
Co-authored-by: Sergey Nuyanzin <sergey.nuyanzin@aiven.io>
@L-Evg L-Evg requested a review from snuyanzin April 17, 2023 06:09
@snuyanzin snuyanzin merged commit 1f9742a into datafaker-net:main Apr 17, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snuyanzin snuyanzin snuyanzin approved these changes

@kingthorin kingthorin kingthorin approved these changes

@panilya panilya panilya approved these changes

@bodiam bodiam Awaiting requested review from bodiam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@L-Evg @bodiam @codecov-commenter @kingthorin @snuyanzin @panilya