Add @group Others to all other tests

[paulbalandan]

Description
Since we have DatabaseLive, CacheLive, SeparateProcess, AutoReview groups already, I think we need a catch-all group for the others for consistency.

Use case: Since there are existing groups already for tests, I find that useful for my refactoring of the phpunit tests to be more efficient. The drawback is that not all tests have groups, which would unnecessarily add logic of excluding groups instead of adding group.

If you have a better term for the other group, I'm all ears.

In a separate PR, I'll add an autoreview test to enforce groups.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

Since we have DatabaseLive, CacheLive, SeparateProcess, AutoReview groups already, I think we need a catch-all group for the others for consistency.

Can you explain more about what makes us happy with the tagging?

I don't get what is consistency, and what is good.
I don't think we need to force a group tag, and we can put multiple group tags on a single test.

I don't understand the meaning of the tag for others.
We can run others with --exclude-group DatabaseLive,CacheLive,SeparateProcess,AutoReview.

[paulbalandan]

This is what I'm trying to do with the phpunit tests:

1. Currently, our tests are run based on matrices (one for PHP version, one for DB driver). Ignoring the PHP version part, for each DB driver we have, we run the full suite of tests (excluding autoreview) for the driver. However, there are tests which do not depend on the DB driver. For example, console command tests do not depend on the DB but we run 5 DB-driver-based tests on those console tests. It is unnecessary and takes time and resources. One run is enough.
2. My solution is to use Github Action's reusable workflows. So, now, my dilemma is to properly define the inputs needed so that the tests can be modularized.
3. Initial solution that comes to mind is use the testsuite found in phpunit.xml.dist:

<testsuites>
      <testsuite name="AutoReview">
          <directory>./tests/AutoReview</directory>
      </testsuite>
      <testsuite name="System">
          <directory>./tests/system</directory>
          <exclude>./tests/system/Database</exclude>
      </testsuite>
      <testsuite name="Database">
          <directory>./tests/system/Database</directory>
      </testsuite>
  </testsuites>

The idea is System testsuite will be run once for each PHP version. The Database testsuite will be run on the existing 2 matrices.
4. However, based on your last PR, there are tests in System testsuite that depends on the DB driver so that's why you put the @group DatabaseLive annotation. This renders the testsuite idea pointless, because I cannot extract those tests and put them inside the Database testsuite.
5. This is where the @group annotation comes to play. Since we have existing groups already as I mentioned, then the reusable workflow that I'll be making should just need to invoke vendor/bin/phpunit --group <GROUP>. BUT not all tests have group annotations.
6. This PR adds the missing group annotations so that it'll be easier to vendor/bin/phpunit --group <GROUP> instead of vendor/bin/phpunit --exclude-group <GROUP1, GROUP2, GROUP3, GROUP4>
7. A test class is not limited to one group. It can contain several groups since PHPUnit allows it.

[kenjis]

Initial solution that comes to mind is use the testsuite found in phpunit.xml.dist:

It seems to me the initial solution is better than tagging to other test files.
It seems difficult to keep adding others tag to new test files.

I put @group DatabaseLive in some command tests, because they need live
database connection.

But they are tests for commands, not databases.
I think we don't need to run them on all databases.
Or if we think we need to run on all databases, it is better to move the test files to Database folder.

[paulbalandan]

It seems to me the initial solution is better than tagging to other test files. It seems difficult to keep adding others tag to new test files.

How is it difficult. You just need one line each. And there's an auto-review to remind you that it needs the group.

I put @group DatabaseLive in some command tests, because they need live database connection.

Running --testsuite System on different DB connections via matrix will defeat the purpose of the segregation.

But they are tests for commands, not databases. I think we don't need to run them on all databases. Or if we think we need to run on all databases, it is better to move the test files to Database folder.

I don't agree. Those tests are there but incidentally they need DB connections, so that's why you group them. No need to move them.

[kenjis]

I see. Okay, tagging is more flexible than using folders.

If we automate it, it would be nice if php-cs-fixer adds it.

I think Others or Other is better.
The group name actually indicates no group.
It is different from other existing group names that mean something.

[paulbalandan]

For php-cs-fixer, there's currently no built-in fixer for that except for a fixer that adds a @covers annotation. I'll check if PHPUnit has an XML configuration for that. If not, I'll try spinning a custom fixer for that.

[MGatner]

Rector could add it pretty easily.

I like "other" or "core" for names. Please don't use "unit tests" on anything that requires infrastructure and is actually an integration test.

Bigger picture... if the real issue is just that these Command tests break the organization: why don't we mock the dependency in those tests specifically (and in anything that "needs" a live connection)?

[kenjis]

other may be good for the name. Because it is completely different from other group names.

Bigger picture... if the real issue is just that these Command tests break the organization: why don't we mock the dependency in those tests specifically (and in anything that "needs" a live connection)?

It seems not flexible. It means all live database tests must be in the Database folder.
But in fact, there are also live tests in Models folder.
On the other hand, Database folder contains tests without live database connections.

What if we need live Cache tests with different versions of Cache servers?

[MGatner]

Are we being too rigid about our folder structure? Ideally tests target a single logical unit (which may span classes). If they are deterministic and self-contained then they are unit tests, if they rely on infrastructure service integration they are integration tests. This seems like a good starting point for a directory structure and baseline grouping, then we can use tags or subfolders to indicate which infrastructure components (database, system clock, cache, etc) are needed "live" as we care about them. Theoretically that would save us from needing an "other" category altogether.

[paulbalandan]

My issue is not limited to command tests. There are others too, like HTTP tests. Those do not need DB connections.

In terms of testing infrastructure, I would say CI4 has a rather unconventional directory structure. My expectations were:

1. Test fixtures would reside solely under tests/Fixtures/, not tests/_support/ or tests/system/**/fixtures/.
2. Unit tests would reside on their PSR-4 compatible paths, which would be src/**/ (or system/**/ in this matter) would just be replaced by tests/**/. Simple substitution like that.
3. Tests that need external services to work would be under tests/Integration/**/
4. End-to-end tests would be under tests/E2E/**/ or tests/EndToEnd/**/
5. Autoreview tests would be in tests/AutoReview/

[kenjis]

In terms of testing infrastructure, I would say CI4 has a rather unconventional directory structure.

Yeah, where does the _support/ come from?

[MGatner]

Yeah, where does the _support/ come from?

The Dark Ages 🙃

I am happy for us to take a pass at restructuring, especially if it is for the sake of improving our test accuracy. I'm reading this book right now and my two big takeaways from the testing section (both of which I am guilty of) are:

1. Tests should not simply repeat class logic
2. Unit tests should target a unit of business logic instead of a class or method

Number 2 gets a little murky when you are testing a framework 🫢 but I think there are still some good takeaways and I would like our structure to encourage those behaviors.

[MGatner]

I'm not going to flip through all of these but I'm on board with the approach.