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

Convert some DateTime and Duration tests to be table driven #313

Merged
merged 4 commits into from
Mar 3, 2023
Merged

Convert some DateTime and Duration tests to be table driven #313

merged 4 commits into from
Mar 3, 2023

Conversation

PiotrLewandowski323
Copy link
Contributor

Fixes #310

I've created the PR, but I'm not sure about 2 things:

  • How to handle the test cases below in table driven style as they use different function to create the object than other test cases.
newDuration(newMockChain(t), nil).InList(time.Second).chain.assertFailed(t)
newDuration(newMockChain(t), nil).NotInList(time.Second).chain.assertFailed(t)
  • I removed some test cases for InList tests, as I believe they were copied from InRange test cases and didn't test any edge cases, e.g.:
value.InList(time.Unix(0, 1234+1), time.Unix(0, 1234+2))
value.chain.assertFailed(t)
value.chain.clearFailed()

value.NotInList(time.Unix(0, 1234+1), time.Unix(0, 1234+2))
value.chain.assertNotFailed(t)
value.chain.clearFailed()

value.InList(time.Unix(0, 1234-2), time.Unix(0, 1234-1))
value.chain.assertFailed(t)
value.chain.clearFailed()

value.NotInList(time.Unix(0, 1234-2), time.Unix(0, 1234-1))
value.chain.assertNotFailed(t)
value.chain.clearFailed()

If any of these should be changed please let me know.

Signed-off-by: PiotrLewandowski323 <lewandowski323@gmail.com>
Signed-off-by: PiotrLewandowski323 <lewandowski323@gmail.com>
@coveralls
Copy link
Collaborator

coveralls commented Feb 27, 2023

Coverage Status

Coverage: 95.02% (-0.2%) from 95.181% when pulling c697d3c on PiotrLewandowski323:issue-310 into 9e6c89d on gavv:master.

@gavv gavv added the ready for review Pull request can be reviewed label Feb 27, 2023
@gavv
Copy link
Owner

gavv commented Feb 27, 2023

Hi, thanks for PR, will review in upcoming days.

newDuration(newMockChain(t), nil).InList(time.Second).chain.assertFailed(t)
newDuration(newMockChain(t), nil).NotInList(time.Second).chain.assertFailed(t)

these two tests can be just removed, they cover a deprecated feature of "unset" duration (when we pass nil to ctor), which will be removed in upcoming releases.

@gavv
Copy link
Owner

gavv commented Feb 27, 2023

I removed some test cases for InList tests, as I believe they were copied from InRange test cases and didn't test any edge cases, e.g.:

Seems legit.

Signed-off-by: PiotrLewandowski323 <lewandowski323@gmail.com>
@gavv
Copy link
Owner

gavv commented Mar 3, 2023

The patch looks good!

Could you please replace map of structs with slice of structs?

Here is recent discussion from chat (BTW welcome to join if you wish)

gavv — 02/27/2023 10:20 PM
@gpjservais I was debugging a test that has cases := map[string]struct {, and found it a bit inconvenient that subtests are executed in different order each invocation (for obvious reasons)

since it's a matter of a few lines, I think it would be nice to switch from maps of structs to slices of structs in future tests

(posting it to chat so that others could see the message too)

gpjservais — 02/27/2023 10:52 PM
Sounds good! I'll make sure to use slices of structs moving forward.

gavv — Today at 2:06 PM
@gpjservais FYI: cc1810b

@gavv
Copy link
Owner

gavv commented Mar 3, 2023

Could you please also rename instance to tc? "tc" stands for "test case" and is used in most table tests in this lib.

@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Mar 3, 2023
Signed-off-by: PiotrLewandowski323 <lewandowski323@gmail.com>
@gavv gavv merged commit e025293 into gavv:master Mar 3, 2023
@gavv
Copy link
Owner

gavv commented Mar 3, 2023

Thanks!!

@gavv gavv removed the needs revision Pull request should be revised by its author label Mar 3, 2023
Rohith-Raju pushed a commit to Rohith-Raju/httpexpect that referenced this pull request Mar 4, 2023
Signed-off-by: PiotrLewandowski323 <lewandowski323@gmail.com>
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.

Convert a few DateTime and Duration tests to table-driven style
3 participants