I believe the more recent syntax is quite a bit more readable. The
most obvious benefit is that it allows for much less duplication.
Note this patch is intentionally only touching tests, so it can't
have any effect on production code.
Change-Id: Ibdde9e37edf80f0d3bb3cb9056bee5f7df8010ee
Statsd-exporter expects timing metrics in ms, and normalizes unit
conversion for scraping by Prometheus.
This gives users of TimingMetric functions for the unit
being measured and recalculates as necessary.
Bug: T364240
Change-Id: Ieb24a5f7230845713e5feaac75e279ed0250b808
get_debug_type() does the same thing but better (spelling type names
in the same way as in type declarations, and including names of
object classes and resource types). It was added in PHP 8, but the
symfony/polyfill-php80 package provides it while we still support 7.4.
Also remove uses of get_class() and get_resource_type() where the new
method already provides the same information.
For reference:
https://www.php.net/manual/en/function.get-debug-type.phphttps://www.php.net/manual/en/function.gettype.php
In this commit I'm only changing code where it looks like the result
is used only for some king of debug, log, or test output. This
probably won't break anything important, but I'm not sure whether
anything might depend on the exact values.
Change-Id: I7c1f0a8f669228643e86f8e511c0e26a2edb2948
get_debug_type() does the same thing but better (spelling type names
in the same way as in type declarations, and including names of
object classes and resource types). It was added in PHP 8, but the
symfony/polyfill-php80 package provides it while we still support 7.4.
Also remove uses of get_class() and get_resource_type() where the new
method already provides the same information.
For reference:
https://www.php.net/manual/en/function.get-debug-type.phphttps://www.php.net/manual/en/function.gettype.php
To keep this safe and simple to review, I'm only changing cases where
the type is immediately used in an exception message.
Change-Id: I325efcddcb58be63b1592b9c20ac0845393c15e2
Mostly used find-and-replace:
Find:
/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?)[\s\*]+/\s*(private|protected|public) (\$[a-z]\w+;\n)((?=\s*/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?))\n|)
Replace with:
\3 \1 \4
More could be done, but to keep this patch reasonably sized, I only
changed the most obvious and unambiguously correct cases.
In some cases, I also removed redundant doc comments on the
constructor, and re-ordered the properties to match the constructor.
Change-Id: I3f8427ae4f5d55177ae18986ef15d84d0e7bf6f4
Mostly used find-and-replace:
Find:
/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?)[\s\*]+/\s*(private|protected|public) (\$[a-z]\w+;\n)((?=\s*/\*[\*\s]+@var (I?[A-Z](\w+)(?:Interface)?))\n|)
Replace with:
\3 \1 \4
More could be done, but to keep this patch reasonably sized, I only
changed the most obvious and unambiguously correct cases.
In some cases, I also removed redundant doc comments on the
constructor, and re-ordered the properties to match the constructor.
Change-Id: I819ed771c915293663856c577a481d607b76ed80
> We lose useful coverage and waste valuable time on keeping tags
> accurate through refactors (or worse, forget to do so).
>
> Tracking tiny per-method details wastes time in realizing (and
> fixing) when people inevitably don't keep them in sync, and time
> lost in finding uncovered code to write tests to realize it was
> already covered but "not yet claimed".
>
> Given all used methods are de-facto and liberally claimed, and
> that we keep the coverage limited to the subject class, this
> maintains the spirit and intent. PHPUnit offers a more precise
> tool when you need it (i.e. when testing legacy monster classes),
> but for well-written code, the class-wide tag suffices.
Ref https://gerrit.wikimedia.org/r/q/owner:Krinkle+is:merged+message:Widen
Change-Id: I28312ced932898a32b53a8dfc1be12ba165c79c4
* Removes testSendGaugeMetric
* Ensure we're checking for calls to timing, gauge, and updateCount
* Tune expectations to exactly how many times we expect the calls
Bug: T370636
Change-Id: I73f05b1876e3afbf0995d5605cb17fa5dc6146d2
In I15d6f100a31fbb98ad (13f556ab3a), we used `updateCount()`
on Gauges when defining the `set()` method for `copyToStatsdAt()`
instead of calling `gauge()`.
The side effect of this is that StatsLib will treat all guages as
counters instead of proper gauge values. This caused an issue with
the `blameStartupRegistry.php` script migration to prometheus format
to break in the way it sends its gauge data to Graphite.
Credits to @Krinkle for the debugging session where we found out that
this was the issue in the library.
Bug: T370636
Co-Authored-by: Krinkle <krinkle@fastmail.com>
Change-Id: I34cf3ce193b354ae732c1d3b0fd3b851d00aeaf0
In 279fd16bab, I made what I thought
was a trivial change to UserAuthorityTest:
(diff slightly modified here for clarity)
- $message = $permissionStatus->getErrors()[2]['message'];
+ $message = $permissionStatus->getMessages()[2];
$this->assertArrayEquals(
$this->getFakeBlockMessageParams(),
$message->getParams()
);
And in 3d92cb2f82, I made what I thought
was also a trivial change to UserAuthority:
(diff slightly modified here for clarity, likewise)
- foreach ( $errors as $err ) {
- $status->fatal( wfMessage( ...$err ) );
- }
+ $status->merge( $tempStatus );
However, it turns out these two pieces of code had vital roles:
* The code in UserAuthority ensured that the final status contains
Message objects instead of key strings + parameter arrays, and thus
does not trigger wikitext escaping in a legacy code path (T368821).
* The code in UserAuthorityTest accessed the internals of the same
status with (now deprecated) getErrors() to check that it indeed
contained a Message object, rather then a key string, which would
cause a test failure due to a fatal error in the code below.
getMessages() returns objects regardless of what's inside the
status, so the test never fails.
Thus I managed to disarm the regression test, and then cause exactly
the regression it was supposed to prevent: block error messages on
Special:CreateAccount have parameters shown as wikitext (T306494).
Restore a foreach loop instead of `$status->merge()` to fix that, and
document why it is there. Change the test so that it actually runs
the code whose behavior it wants to verify, instead of a related but
different method, hopefully making it more resilient against future
developers.
(I found the bug because the test started failing with the refactoring
I'm trying to do in I625a48a6ecd3fad5c2ed76b23343a0fef91e1b83.)
Bug: T306494
Change-Id: I7601fc51702cb33ef9d2b341ea555dc230d31537
Follow-up to f18362ccce.
While working on I625a48a6ecd3fad5c2ed76b23343a0fef91e1b83,
I learned that Stringable objects are sometimes also used
as normal params, intended to be stringified when rendering.
For example, see `this->target` in the UnblockUser class.
That code uses Message objects, so it's not causing trouble,
but converting them to MessageValue would emit warnings.
I didn't actually intend to deprecate that, and it won't
cause issues with JSON-serializability (assuming we stringify
the objects in time), so tweak how the warnings are emitted.
Change-Id: I26dfd4f1ac8ed08a422692de4e39d072242c08df
https://gerrit.wikimedia.org/r/q/owner:Krinkle+is:merged+message:Widen
> Given all called methods are de-facto and liberally claimed, and
> that we keep the coverage limited to the subject class, it maintains
> the spirit and intent by listing the class explicitly instead.
>
> PHPUnit offers a more precise tool when you need it (i.e. when testing
> legacy monster/god classes), but for well-written code, the
> class-wide tag is exactly what you want.
>
> We lose useful coverage and waste valuable time on keeping tags
> accurate through refactors (or worse, forget to do so).
> Tracking tiny per-method details wastes time in realizing (and
> fixing) when people inevitably don't keep them in sync, and time
> lost in finding uncovered code to write tests to realize it was
> already covered but "not yet claimed".
While at it, also fix PHPUnit warnings in CssContentHandlerIntegrationTest
and JavaScriptContentHandlerIntegrationTest about not having any
`@covers` annotations.
Change-Id: I5afd9fe0bca0fa86cc096f6e5e79f2ba1cfbfa77
Since I72c5e6f86b7f081ab5ce7a56f5365d2f75067a78 it is part of the
contract of ContentRenderer::getParserOutput() that the render ID (and
other cache parameters) will be set when it returns.
(ContentHandler::getParserOutput() can set them even earlier if it has
custom content-based overrides.) We had a lot of temporary
backward-compatibility code "later" in the parse process to try to close
the barn door if some code path "forgot" to set them, but these are
unnecessary now.
This patch removes that backward-compatibility code in ParsoidParser;
there is similar remaining code in ParserCache etc. which can be
addressed in follow ups.
(For compatibility we do have to temporarily copy the render ID code
inside ParsoidOutputAccess::parseUncachable, but that class is
deprecated and will be removed.)
The HtmlOutputRendererHelper path which used to call
ParsoidParser::parseFakeRevision() is now replaced with a codepath that
goes through RevisionRenderer. In order to maintain the same behavior
of the ParsoidHandler, we have also added 'useParsoid' handling to the
JsonContentHandler. This support can perhaps be deprecated eventually.
Bug: T350538
Change-Id: I0853624cf785f72fd956c6c2336f979f4402a68f
Why:
* When temporary accounts are disabled, no new temporary
accounts should be created either on edit or through
a CentralAuth autocreation.
* UserNameUtils::isUsable is used to prevent the
CentralAuth autocreation making the temporary account
username not usable for login or account creation.
* However, this check to determine if the name is
reserved by the temporary user system still considers
the name as a valid temporary account if the system
was once enabled (but since disabled).
* This is a bug, and instead if the feature is disabled
the username should be considered unusable. This then
prevents the CentralAuth autocreation on a wiki which
knows the feature but has it disabled.
What:
* Update UserNameUtils::isUsable to return false if
the name is reserved by the temporary user system
and TempUserConfig::isEnabled returns false.
* Test this new behaviour with a unit test.
Bug: T370513
Change-Id: I467c39538796a660a7417397c99928a1a25007bc
Instead of creating a half-initialized helper and later calling ::init,
provide all the information necessary for the helper in the constructor.
This is facilitated by the fact that there already exists a factory
class, PageRestHelperFactory, which holds all the services required.
This affects:
* HtmlOutputRendererHelper::init()
* HtmlMessageOutputHelper::init()
* HtmlInputTransformHelper::init()
Change-Id: I1e1213597c6be012f2bc024c2b370c968ff3b472
Why:
* When temporary accounts are known, the match patterns need to
be available. If no match patterns are set, the match pattern
is set to an array containing the generate pattern. Therefore
the generate pattern must also be available if temporary
accounts are known.
* This was not the case before this commit, and certain settings
caused an error to be thrown from isTempName.
What:
* Use the generate pattern for the match patterns if temp accounts
are known and no match patterns are set.
* Update the tests to use a generate pattern but no match pattern
(the same as the default config).
Change-Id: Iadfbed40b14378a9309e8d7afab6064e25480514
A single test function can have multiple @dataProviders. No problem.
A @dataProvider can also be used by multiple test functions. No
problem. No need to duplicate such large pieces of code when it's
identical anyway.
Change-Id: I5aea36304ec2d1666ff2334ba883df07a70c15c5
This removes the last use of ParsoidOutputAccess in core, allowing it
to be deprecated and eventually removed.
Bug: T367074
Bug: T317018
Change-Id: Ica2c880e2e7c2b126aaea66a3e4be460b3f2234f
Many unit tests needs adjust, because the ->with() call no longer
matched and using expression objects for the matching repeats the same
code between test and real implementation.
Bug: T361023
Change-Id: I6f8dbd12d7135ee28ad5b1eabb58d1354d1591d3
This mistake happened in If455682. It's a bit funny. There are two
providers that are 100% identical anyway. The old provider was copied
but the new test accidentally uses the old provider. Which is totally
fine. They are identical anyway.
Change-Id: I473a96769f700a0f601dce16a86b1dc4ff8f4297
In T361190 and Quibble 1.9.0, we introduced parallel execution of
PHPUnit tests to speed up the CI jobs. The existing implementation
is purely Python/Quibble, and cannot directly be used by developers
locally. With this patch, we re-implement the parallel test
execution already implemented in CI as a composer task so that the
parallel tests can be run locally.
The `phpunit:parallel:extensions` command expects to be run after
`phpunit:prepare-parallel:extensions`, and expects to find 8 test
suites with the names `split_group_X` (for X in 0 through 7) in the
PHPUnit configuration file. 8 here is currently a hard-coded number
that corresponds to the number of parallel test executions we need
to saturate the CPU of a 4-core developer machine, and experimentally
leads to a good speed-up versus the serial execution.
When this command runs, it forks to launch 8 parallel processes,
each running one of the `split_group_X` suites. The parent process
waits for the children to complete, buffers the output, collects the
exit statuses, then dumps the buffered output and exits with a
non-zero status if any of the child processes failed (i.e. if there
were test failures).
We introduce `phpunit:prepare-parallel:default` as a complement to
`phpunit:prepare-parallel:extensions`, and the two commands
`phpunit:parallel:database` and `phpunit:parallel:databaseless`.
This creates four possible combinations - two different test suites,
and two different test groups. This is a similar setup to that which
we have in CI - the Database and non-Database tests are run in
separate groups, and some jobs use the `extensions` suite while
others just use the default suite.
The `phpunit:parallel:...` commands will fail with a helpful message
if no `split_group_`s are found in the active PHPUnit configuration.
To help test whether the split test runs are really running all the
tests in the suite, we generate and store the PHPUnit results cache
file. Comparing the results cache files from linear versus parallel
runs should tell us if all the tests have been executed.
Bug: T365976
Change-Id: If106802f08edd5d4c841bb7970c69b88ab3bb39b
ParamValidator and TypeDef could so far assume that data coming from the
client was always a string. Since we are now using the same code to
validate fields coming from a JSON request body, we may encounter arrays
and other non-string values in places where strings are expected. In
that case, we should fail gracefully instead of crashing with a type
error.
Change-Id: I8a3f0c3299d36e1e93e0114abc0d1d15bac7b5f8
