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

feat: add a more robust set of tests #85

Merged
merged 25 commits into from
Oct 21, 2024
Merged

Conversation

hanoii
Copy link
Contributor

@hanoii hanoii commented Oct 8, 2024

Adding something more useful in terms of test coverage, for now this adds:

Different drupal version and a smaller set of tests are run by GHA strategy matrix

  • a very bear dummy drupal module instead of relying on keycdn
  • tests if the module has no composer ddev poseer should still work
  • with both d10 and 11 of core supported versions tests that:
    • it installs
    • ddev poser works
    • ddev symlink-project works
    • the core version install is the one specified
    • php tests comamnds are available
    • node commands are available

What I like to add in this initial approach is that eslint and stylelint actually does something, but that can be added later.

The tests will fail until #81 and #80 are accepted. They are in, tests should now be green

@hanoii
Copy link
Contributor Author

hanoii commented Oct 10, 2024

rebased, and try a fix as something was failing

@hanoii
Copy link
Contributor Author

hanoii commented Oct 10, 2024

tests are green

@weitzman
Copy link
Collaborator

LGTM.

I have done it in a while, but I want to make sure our tests can still pass locally. Could you please check that and document at the bottom of the README that command to run?

@hanoii
Copy link
Contributor Author

hanoii commented Oct 11, 2024

@weitzman I did run the tests manually several times but in testing this again I noticed some potential issues by not using the new TEST_DRUPAL_CORE env var.

I fixed this and now just running it locally works and uses the core default.

Added to the readme, latest version can be read on https://github.com/hanoii/ddev-drupal-contrib/blob/tests-rehaul/README.md#add-on-tests

EDIT: Added some small tweaks to the test

@hanoii
Copy link
Contributor Author

hanoii commented Oct 11, 2024

Still doing some work

@hanoii
Copy link
Contributor Author

hanoii commented Oct 11, 2024

@weitzman it's ready for review, I decided to use git submodule for bash bats, see ddev/github-action-add-on-test#36, but no change on the ddev bats github action is needed hanks to disable_checkout_action.

My rationale for this is that it's a lot easier to install and to depend on things like bats-assert. Updated the README but the gist to run this locally is:

git submodule update --init
./tests/bats/bin/bats tests

Hope you like it, it was quite a bit of work getting it right!

EDIT: commands simplification

@weitzman
Copy link
Collaborator

Any feedback, @rfay @tyler36?

Copy link
Contributor

@tyler36 tyler36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well and as intended.
I like the detailed test output.

I've had bad expereience with git submodules before so I tend to avoid them, however, it worked locally for me here. And more importantly didn't conflict with my local bats brew setup.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any time somebody goes to the crazy effort to expand and improve tests I have to be happy!

I'm pretty sure we don't need specialty handling for d10/d11.

I wouldn't have put the bats extensions in git submodules because I've always hated them, but the README makes it sound easy to activate them.

tests/full.bats Show resolved Hide resolved
Co-authored-by: tyler36 <tyler36@users.noreply.github.com>
@hanoii
Copy link
Contributor Author

hanoii commented Oct 16, 2024

@rfay

I'm pretty sure we don't need specialty handling for d10/d11.

It's true that currently there's no difference, but... I did experience an issue: #82, while not with the functionality, the way it was suggested to install yarn, didn't work as is with d11.

I figured it wasn't bad that we have both cores. And with that, I am also testing the ability to change cores and making sure that's being picked up.

@hanoii hanoii requested a review from rfay October 16, 2024 11:45
@hanoii
Copy link
Contributor Author

hanoii commented Oct 16, 2024

Most suggestion were addressed but one which I followed up. Thanks

@hanoii hanoii requested a review from tyler36 October 16, 2024 12:10
@rfay
Copy link
Member

rfay commented Oct 16, 2024

My suggestion of ddev config --auto was based on the assumption that the code is already laid down; it works well in that situation. DDEV detects docroot and subversion of drupal and does the right thing with corepack, etc. So I wasn't saying "don't test both d10 and d11", I was saying that if you like it, you can use DDEV's auto-config that detects and makes the right choices.

@hanoii
Copy link
Contributor Author

hanoii commented Oct 16, 2024

but the code is not already layed out in the context of that test, as we are testing adding the core later on as part of ddev poser

@hanoii
Copy link
Contributor Author

hanoii commented Oct 16, 2024

I was going to add some thoughts on the git submodules discussion, only because I feel it's interesting... but added it to ddev/ddev-addon-template#62 instead

@rfay
Copy link
Member

rfay commented Oct 16, 2024

If the code isn't there yet, then using ddev config to add hints of what's going on is a great technique, as in the quickstarts, which do the config before the composer create-project. https://ddev.readthedocs.io/en/stable/users/quickstart/#drupal

@weitzman weitzman merged commit 78380b6 into ddev:main Oct 21, 2024
6 checks passed
@hanoii hanoii deleted the tests-rehaul branch October 21, 2024 11:59
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.

4 participants