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

Change 'tests' in definitions to be more generic #7

Closed
ppeble opened this issue Oct 15, 2016 · 10 comments
Closed

Change 'tests' in definitions to be more generic #7

ppeble opened this issue Oct 15, 2016 · 10 comments
Assignees

Comments

@ppeble
Copy link
Member

ppeble commented Oct 15, 2016

Right now all of the 'tests' are ruby. This has to change if we want to use these definitions in other languages. We need a format that can express all of the tests that exist today in a YAML format.

My proposal:

tests:
  - name: "New Year's Day"
    year: 2015
    month: 1
    day: 1
    regions: ['us']
  - name: Columbus Day
    year: 2015
    month: 10
    day: 10
    options: ['observed']
    regions: ['us']
  - name: "Phil's Birthday!"
    year: 2015
    month: 8
    day: 8
    options: ['informal']
    regions: ['us']
  - name: "Something"
    year_range: 2014..2020
    month: 1
    day: 1
    options: ['observed']
    regions: ['us', 'uk']
  - name: "Else"
    year: 2016
    month: 6
    day: 15
    valid: false
    regions: ['us']

The region should be implicit based on the current file being tested.

I'm going to try to do this now and the modify the ruby repository to generate based on these new definitions. If I do it right then the generated classes will look identical to the existing ones.

@ttwo32
Copy link
Member

ttwo32 commented Oct 15, 2016

👍

@ppeble ppeble self-assigned this Oct 22, 2016
@ppeble
Copy link
Member Author

ppeble commented Oct 22, 2016

Slight modification here. Instead of found I am going with valid: false. The found usage just....felt weird. I'm not sure valid makes much more sense but we'll stick with it for the moment.

@ppeble
Copy link
Member Author

ppeble commented Oct 22, 2016

Aaaaannnnnnddd I've hit my first real problem. The above setup won't work unless I also include the region. We can't assume based on the file because of subregions. We'll need to add in a 'region' key. ☹️

@iGEL
Copy link
Contributor

iGEL commented Oct 24, 2016

I was thinking about this. First of all, I would suggest to have a date instead of day, month & year. And I also don't really like the valid thing. Usually, tests are written in a "given these inputs, I expect these results". I think, that could be expressed more clearly here, maybe like this:

tests:
  - given:
      date: 2015-01-01
      regions: ["us"] # could be optional unless you want to have something different than the files main locale
    expect:
      name: "New Year's Day"
      holiday: true
  - given:
      date: 2015-10-10
      observed: true
    expect:
      name: Columbus Day
  - given:
      date: 2016-06-15
    expect:
      holiday: false

That would generate these tests:

  • 2015-01-01 is a holiday
  • 2015-01-01 is "New Year"s Day"
  • observed 2015-10-10 is Columbus Day
  • 2016-06-15 is not a holiday

@ppeble
Copy link
Member Author

ppeble commented Oct 24, 2016

Ooooh, I like this a lot! I have to agree regarding my current proposal. I've completed two definition file conversions with my format and I already don't like it. I've slowed down because...it just feels bad. Don't know how to describe it but I don't like it.

I'm at work now but let me chew on this and I'll get back to you. Thank you so much for the input.

@ppeble
Copy link
Member Author

ppeble commented Nov 6, 2016

I wrote the initial validator for the updated format and did the ar region: https://github.com/ptrimble/definitions/blob/issue-7/ar.yaml#L74-L193

I made regions and holiday (to be true/false) required for ease of use and clarity. What do we think?

@ppeble
Copy link
Member Author

ppeble commented Nov 27, 2016

I have been converting test defs and I think I'm going to assume holidays:true unless specified otherwise. That'll make this a lot easier to read.

@ppeble
Copy link
Member Author

ppeble commented Feb 23, 2017

I made a small update to the proposed structure to allow for multiple dates to be passed in the given block. This way if we have a situation where we are testing for multiple years worth of dates for the same holiday we are not repeating the block of YAML.

Example:

  - given:
      date: ['2011-08-15', '2012-08-15']
      regions: ["pl"]
    expect:
      name: "Święto Wojska Polskiego"

@iGEL
Copy link
Contributor

iGEL commented Feb 26, 2017

Awesome!

@ttwo32
Copy link
Member

ttwo32 commented Feb 26, 2017

@ptrimble That's good and useful!! 👍

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

No branches or pull requests

3 participants