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

test that yield is given number of seconds #6

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

jjb
Copy link
Contributor

@jjb jjb commented Sep 24, 2021

Nothing currently covers this behavior

@ioquatix
Copy link
Member

This looks good to me but I feel a little bit odd about using a string. But it also works pretty well… what do you think?

@jjb
Copy link
Contributor Author

jjb commented Sep 26, 2021

My goal was to do something "realistic" which does something with the value, to test that the return value is what the block returns and also that the block has access to the param.

If we don't use a string, what alternative do you have in mind? one of these?

    assert_equal 5, Timeout.timeout(5){|s| s }
    assert_equal 6, Timeout.timeout(5){|s| s+1 }

@ioquatix
Copy link
Member

I would say there are two tests:

  • That the time is provided to the block as an argument.
  • That the value of the block is returned.

The biggest issue I see with the string formatting is time precision, but as long as we use an integer, it would be okay? But I'm not sure we can guarantee the value of the timeout argument? In the future we might want a more precise measurement of time than just the numeric value.

@jjb
Copy link
Contributor Author

jjb commented Sep 26, 2021

ah, so you are saying that involving the string interpolation makes the test brittle, because one day the code might, say, run to_f on s, and then the string will be "5.0 seconds" and the test will erroneously break?

what do you think of one of these?

    assert_equal 5, Timeout.timeout(5){|s| s }
    assert_equal 6, Timeout.timeout(5){|s| s+1 }
    assert_equal [5, :ok], Timeout.timeout(5){|s| [s, :ok] }
    x = rand(1..10); assert_equal x, Timeout.timeout(x){|s| s }

although... those might all have the same problem... depending on if the test environment does a flexible equals 5==5.0

@ioquatix
Copy link
Member

I think as long as we consider the issue, I'm fine with whatever you ultimately propose.

@jjb
Copy link
Contributor Author

jjb commented Sep 27, 2021

Okay, I went with the array

@jjb
Copy link
Contributor Author

jjb commented Sep 27, 2021

I don't know why CI isn't running... a rebase and another push triggered it now

@ioquatix ioquatix merged commit ec5a614 into ruby:master Sep 27, 2021
@jjb jjb deleted the test-for-block-param branch September 27, 2021 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants