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

issue-7: modify all tests to be in pure YAML rather than raw ruby #26

Merged
merged 24 commits into from
Jul 25, 2017

Conversation

ppeble
Copy link
Member

@ppeble ppeble commented Feb 27, 2017

Fixes #7

Title says it all. What do we think? I would appreciate a spot check that I didn't leave any obvious mistakes. I automated the generation of the new YAML based off of the old ruby but...there was still a lot of manual intervention. I feel that the chances for errors are high.

I'm not going to merge this for a bit. Before I do that I need to get a branch up on the Ruby side to generate our tests from this YAML rather than just shoving the raw ruby that exists today. I don't know how long that will take (with my life circumstances, baby and all that).

I'm happy with how this turned out! 😄

@ppeble
Copy link
Member Author

ppeble commented Mar 1, 2017

Note for myself: I need to update the SYNTAX doc to reflect the new 'tests' rules.

@ppeble
Copy link
Member Author

ppeble commented Jul 13, 2017

@ttwo32 I would like to merge this in (once I fix the conflicts) so that it becomes our defacto way of doing tests going forward. Fixing conflicts has been very difficult for me and I would prefer that we just merge it, which will force me to finish the changes on the other repo.

The downside to this approach is that we would not be able to do another release until I finish the test generation on the holidays repo but I think I'm basically done. Just fixing minor issues. And I think this is the pressure I need. What do you think?

@ttwo32
Copy link
Member

ttwo32 commented Jul 13, 2017

I think this is necessary to use these definitions in other languages.
And we need to update the SYNTAX doc to reflect the new 'tests' rules.
But... don't worry!! I will help you to .:)

@ppeble
Copy link
Member Author

ppeble commented Jul 13, 2017

Great! Thank you @ttwo32. I would ask then that we do not merge any new definitions changes until this one is done. I'm almost done with the final bit of US conflicts and after that I'll make the necessary changes to the SYNTAX (thank you for reminding me!).

Hopefully I'll have that handled today or tonight, at which point I'll put up the 'final' version of this for you to look over and I'll switch my attention to a release for the latest defs.

@ppeble
Copy link
Member Author

ppeble commented Jul 14, 2017

Had to do a few merges because the changes were so big. I know this isn't as elegant as cleaner commits but...I'm just so tired of keeping these up to date. 😄

I'm going to get the release out now and then do the SYNTAX bit.

@ppeble
Copy link
Member Author

ppeble commented Jul 16, 2017

@ttwo32 Done with SYNTAX update. 👍

@ppeble
Copy link
Member Author

ppeble commented Jul 21, 2017

@ttwo32 What do you think? Look good in the SYNTAX file?

@ppeble ppeble force-pushed the issue-7 branch 5 times, most recently from 627fa55 to ed25c81 Compare July 22, 2017 03:47
@ppeble ppeble force-pushed the issue-7 branch 6 times, most recently from 9b1b723 to a168bbb Compare July 22, 2017 06:54
One or the other of the `expect` keys is required. If you do not specify a `name` then you should set `holiday: false`.

Here are some more examples. First example shows multiple dates, multiple regions, additional options, and an expectation that the result will be the named holiday.

Copy link
Member

Choose a reason for hiding this comment

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

These examples are good.:)


The format is a straightforward 'given then expect'. Here is a simple example:
Copy link
Member

Choose a reason for hiding this comment

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

Good.It,s easy to write test by 3 sections.

@ttwo32
Copy link
Member

ttwo32 commented Jul 22, 2017

@ppeble Looks good to me!! I really appreciate your hard work.:)

@ttwo32
Copy link
Member

ttwo32 commented Jul 22, 2017

Woo, Why ruby 2.1.0 test failed?

@ppeble ppeble force-pushed the issue-7 branch 3 times, most recently from e47262c to fbf1ac1 Compare July 24, 2017 23:07
@ppeble
Copy link
Member Author

ppeble commented Jul 24, 2017

@ttwo32 I would like your permission to merge this to master, even though the ruby 2.1.0 tests are failing. We know it is already fixed in master and I don't want to have to either rebase or merge a new commit in just to make the PR fully pass.

EDIT: nevermind! I guess I already got it? Not sure when I did that. Either way, this is good!
EDIT2: Oohhhh, it's doing the merge for the test, which pulls in the updated travis yaml. That explains that!

Other than that...this is good! Basically done and ready to go. Here is the PR to use the new format in the ruby side: holidays/holidays#282

If you are okay with this merge, I will go to master and make sure it builds. Then I will change the ruby PR to point to master. Then we can have final signoff on the ruby side. Thanks!

@ppeble ppeble merged commit aac7cae into holidays:master Jul 25, 2017
@ppeble ppeble deleted the issue-7 branch July 25, 2017 15:07
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.

None yet

2 participants