Navigation Menu

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

Added seeRequestTimeIsLessThan function #132

Merged
merged 5 commits into from May 3, 2021
Merged

Added seeRequestTimeIsLessThan function #132

merged 5 commits into from May 3, 2021

Conversation

TavoNiievez
Copy link
Member

@TavoNiievez TavoNiievez commented Apr 24, 2021

Code Example:

	$I->amOnPage('/dashboard');
	$I->seeRequestTimeIsLessThan(40.5);

@Naktibalda
Copy link
Member

The big question is do we want to add performance testing capability to Codeception.
The big problem that comes with it is that code coverage makes PHP much slower, so it would be a good idea to disable this function if code coverage is turned on.

I think that method name could be improved, but I hope that @ThomasLandauer can offer some better names.

The choice of time unit is non-obvious and it would be better to tell what it is in the method name, e.g. seeRequestElapsedTimeInMillisecondsIsLessThan.

What is the expected behaviour of this assertion if /dashboard page redirects using Location header - measure execution time of the last request or of all requests made since amOnPage?

@TavoNiievez
Copy link
Member Author

TavoNiievez commented Apr 25, 2021

The big question is do we want to add performance testing capability to Codeception.

It is not my intention to add a whole layer of utilities and assertions for that. But I think it would be nice if I at least have an assertion for that, if I don't want to install a specific package for performance testing.

The big problem that comes with it is that code coverage makes PHP much slower, so it would be a good idea to disable this function if code coverage is turned on.

For this specific case, that does not apply. I use Symfony's TimeDataCollector which is already collecting that information. All I do is get it and make an assertion with it.

To disable it, and speed up the test or code coverage, you would have to disable or uninstall the Profiler, which would make most of the assertions of this module stop working.

I think that method name could be improved

Me too :)

I would just like to add that I chose that name to be consistent with this:

	# Symfony\Component\HttpKernel\DataCollector\TimeDataCollector L102-L107

    /**
     * Gets the request elapsed time.
     *
     * @return float The elapsed time
     */
    public function getDuration()

So an alternative could be:

$I->seeRequestDurationLessThan(40.5);

The choice of time unit is non-obvious and it would be better to tell what it is in the method name, e.g. seeRequestElapsedTimeInMillisecondsIsLessThan.

I thought about that, but it would also be strange to talk about request time in seconds, for example. In the majority of cases they do not usually take that long. However I agree that it would be even better if the name of the method directly gives me that information... (if it didn't get too long in the process).

So What if we call the parameter '$expectedTimeInMilliseconds' instead?

What is the expected behaviour of this assertion if /dashboard page redirects using Location header - measure execution time of the last request or of all requests made since amOnPage?

Only the time of the last request is taken into account (this seems relevant enough to be added to the docblock).

@Naktibalda
Copy link
Member

To disable it, and speed up the test or code coverage, you would have to disable or uninstall the Profiler, which would make most of the assertions of this module stop working.

My point wasn't that your assertion slows down code coverage, it was another way around. For example, if some request takes 35 milliseconds in normal circumstances and you set your assertion to 40 milliseconds to get warning if it becomes slower, with code coverage, request would take a few times longer, e.g. 100 milliseconds and your assertion would fail.

@TavoNiievez
Copy link
Member Author

oh yeah, you're right.

But I'm not sure; I wouldn't want Codeception to disable my assertions automatically by default in those cases.

I would prefer if I explicitly have to add a configuration in my suite.yml file if I want, for example, that if I have code-coverage activated, mark those tests as incomplete (markTestIncomplete) or directly ignore them (markTestSkipped) .

If you enlighten me on how to know if the code coverage is active I could write that code.

@Naktibalda
Copy link
Member

There is no way to know if code coverage is active in Codeception, but PHPUnit 10 introduced static method for that.

@ThomasLandauer
Copy link
Member

No matter how, but it would be nice if the final result was shorter than seeHttpRequestWasFasterThanTheLimitWhichOurManagementDidSetToTheFollowingNumberOfMilliseconds() ;-)

OK, back to work :-)

  • Unit inside the name is (a) too long and (b) very uncommon - with (b) probably being a consequence of (a). So +1 for the idea of communicating it in the argument's name (which will get more important in the future due to PHP 8). But I'd say that $ms is enough here, cause that says it all.
  • "Only the time of the last request is taken into account" This means that redirects are not counted? Anyway, I would rather not include "Latest" in the function name, since (a) redirects are a bad practice anyway ;-), and (b) they usually don't take long, so it makes not much of a difference - right? So I'd say explaining this in the docblock is enough.
  • "Duration" doesn't add any value over "Time" - it just sounds more complicated ;-) And "time" is also the term used in Symfony's web toolbar & profiler.
  • Most existing functions do contain a verb, even if they are long (e.g. seeResponseCodeIsClientError)

So in total, I'm suggesting:
seeRequestTimeIsLessThan(ms:50)

Just about "Request" I'm not 100% convinced. Is "request time" a commonly used term? What about "ResponseTime"?
What does it actually measure? The time the server takes for internal processing, until it starts sending the HTTP string through the wire?

@TavoNiievez I think what Naktibalda means here:

The big problem that comes with it is that code coverage makes PHP much slower, so it would be a good idea to disable this function if code coverage is turned on.

... is that we/you should not try to solve this in the code, but rather just tell people (in the docs) to manually turn off code coverage. @Naktibalda right?

@TavoNiievez TavoNiievez changed the title Added seeRequestElapsedTimeLessThan function Added seeRequestTimeIsLessThan function Apr 25, 2021
@TavoNiievez
Copy link
Member Author

TavoNiievez commented Apr 25, 2021

he time the server takes for internal processing, until it starts sending the HTTP string through the wire?

As far as I know what getDuration() returns is the time it takes for the server to resolve (internally) a request, hence the 'Elapsed'.

btw, are you sure of $ms? Sometimes you say that abbreviations in general are bad practice...
(In that case, $realTime also needs to be renamed.)

@Naktibalda
Copy link
Member

Parameter name doesn't make test code longer, so it could be changed to $milliseconds or $expectedMilliseconds or even $expectedTimeInMilliseconds :)

The time the server takes for internal processing, until it starts sending the HTTP string through the wire?

As far as I know what getDuration() returns is the time it takes for the server to resolve (internally) a request, hence the 'Elapsed'.

It is time since kernel start, so I think that this assertion could be affected by setting rebootable_client parameter to false.

@ThomasLandauer
Copy link
Member

btw, are you sure of $ms? Sometimes you say that abbreviations in general are bad practice...

Yeah, you caught me here ;-) I suggested this at #66
The difference to me is: Things like "attribs" and "em" are programmers' slang. "ms" is the "official" short version of this unit, like "kg".

Parameter name doesn't make test code longer

That's certainly right for today. But due to PHP 8, the best practices might change, and in 5 years it could be commonplace to write foo(length: 7). Don't you think?
However, it's not that important - so if you prefer, take $milliseconds ;-)

@ThomasLandauer
Copy link
Member

BTW: Is it really necessary to use a float (instead of int) for the milliseconds?

BTW2: I would omit "expected" from $expectedMilliseconds, since it's clear that you cannot pass the actual milliseconds ;-)

@TavoNiievez
Copy link
Member Author

TavoNiievez commented Apr 26, 2021

Is it really necessary to use a float (instead of int) for the milliseconds?

getDuration() returns float, it makes sense to compare it to another float. There is no need to restrict the decimal point in this case.

BTW2: I would omit "expected" from $expectedMilliseconds, since it's clear that you cannot pass the actual milliseconds ;-)

But due to PHP 8, the best practices might change, and in 5 years it could be commonplace to write foo(length: 7)

I kept that in mind, however, the main use of named arguments at the moment is to allow arguments to be passed in a different order than defined in the function. Most of the cases when there are things like this: $this->someMethod([], [], [], [], null, null, $myArgument). So, I think it can stay that way for now.

@ThomasLandauer
Copy link
Member

getDuration() returns float, it makes sense to compare it to another float.

But then, if you do seeRequestTimeIsLessThan(50), PhpStan will give you a warning about the wrong type, right? So maybe you could accept float|int and then do something like:

$ms  = is_int($ms) ? (float) $ms : $ms;

Or (even better) cast the output of getDuration() to int, cause nobody will ever setup a limit in fractions of milliseconds ;-)

@TavoNiievez
Copy link
Member Author

okay, allowing integers as well as floats does sound good. I will make the change soon.

@TavoNiievez
Copy link
Member Author

TavoNiievez commented May 1, 2021

It is time since kernel start, so I think that this assertion could be affected by setting rebootable_client parameter to false.

Effectively, that's right.
If you have rebootable_client: false set, this assertion stops working if the page performs redirects or if you access multiple pages using amOnPage (or similar) in a row:

Fail The Profile is needed to use the 'seeRequestTimeIsLessThan' function.

Any ideas for documentation of this behavior?

ThomasLandauer added a commit to ThomasLandauer/module-symfony that referenced this pull request May 2, 2021
@ThomasLandauer
Copy link
Member

Any ideas for documentation of this behavior?

My suggestion is https://github.com/TavoNiievez/module-symfony/pull/1 :-)

@TavoNiievez
Copy link
Member Author

Thanks @ThomasLandauer :)
Ok I think this is ready to merge.

@TavoNiievez TavoNiievez added this to the 2.0.3 milestone May 2, 2021
@TavoNiievez TavoNiievez merged commit 828acaa into Codeception:master May 3, 2021
@TavoNiievez TavoNiievez deleted the seeRequestElapsedTimeLessThan branch May 3, 2021 13:35
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

3 participants