Skip to content

meetup: grouping testcase and update descriptions #978

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

Closed
wants to merge 2 commits into from

Conversation

jiippoozz
Copy link

This PR closes #974 by grouping related test cases together and updating descriptions to be more helpful.

I haven't yet finished all test cases. I open the PR to you first for some feedback (if any).

@Insti
Copy link
Contributor

Insti commented Oct 29, 2017

Thanks for taking this on ❤️
I'll leave some review comments as soon as I get a chance.

@Insti Insti changed the title meetup #974 : grouping testcase and update descriptions meetup: grouping testcase and update descriptions Oct 30, 2017
"dayofweek": "Sunday",
"dayofmonth": 14
},
"description": "test -teenth",
Copy link
Contributor

Choose a reason for hiding this comment

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

Grouping the tests this way is good 👍
It may not end up in the final version but it is a good way to see which tests exist and what the logical groupings are.

"description": "monteenth",
"cases": [
{
"description": "testcase 1 : May 2013",
Copy link
Contributor

Choose a reason for hiding this comment

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

testcase 1 : May 2013

What is this describing?
That it is "testcase 1" can be determined by the array ordering, so is unnecessary in the description.
Why are we testing May and not June? Why are we testing 2013 and not 2014?

"dayofmonth": 19
},
{
"description": "testcase 3 : September 2013",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there 3 tests?
What is different about them?
Why have we not tested June and July?

"month": 5,
"week": "first",
"dayofweek": "Tuesday",
"dayofmonth": 7
Copy link
Contributor

@Insti Insti Oct 30, 2017

Choose a reason for hiding this comment

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

It can be constructed, but none of these values match the input format in the description:

the first Monday of January 2017

It may be that the cases should look something like:

"description": "First Monday of a month that starts on Sunday",
"property": "date",
"input": "First Monday of January 2017",
"expected": { "year": 2017, "month": 1, "day": 2 }

@Insti
Copy link
Contributor

Insti commented Oct 30, 2017

Thanks @jiippoozz this is a great start.
I suspect there will be a few iterations required on this until we arrive at a good solution. This is due to the existing file being quite different from what I'd like to see, certainly nothing that is your fault ❤️

The TDD process should be able to guide us toward a minimal set of tests that demonstrate all the required functionality.

Have you already implemented this exercise yourself?
(This is not strictly necessary but might be helpful.)

@Insti
Copy link
Contributor

Insti commented Oct 30, 2017

I've submitted a PR (#982) to help clarify the description for this problem, this might effect which test cases are needed and their formatting.

@Insti
Copy link
Contributor

Insti commented Nov 23, 2017

Are you still working on this @jiippoozz ? Do you need any help?

@ErikSchierboom
Copy link
Member

@jiippoozz Small bump. Are you still working on this? If so, you should probably rebase this PR on the latest master, as there has been a small change to the structure of the test cases.

@ErikSchierboom
Copy link
Member

The last work that was done on this PR was over two months ago and there has not been any response to my request for information in the comment above. Hence I think we can probably close this PR. Any objections?

@ErikSchierboom
Copy link
Member

I'm gonna close this due to inactivity. If you (@jiippoozz) would at some point return to working on this, you're very much welcome.

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.

meetup: Test descriptions are unhelpful.
3 participants