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

Internal: Backport AvoidCleanupTrait to 11.9.x #3469

Merged
merged 6 commits into from Jul 24, 2023

Conversation

robertragas
Copy link
Contributor

@robertragas robertragas commented Jul 22, 2023

IMPORTANT:
Should only be merged in 11.9.x

Problem

In 634a6c6 a change has been added using the AvoidCleanupTrait which has been introduced in https://github.com/goalgorilla/open_social/pull/3393/files but this is got introduced in 11.10.x and higher so the tests will fail in 11.9.x as it cannot be found.

Solution

Backport AvoidCleanupTrait #3393

Issue tracker

Internal

Theme issue tracker

[Required if applicable] Paste a link to the drupal.org theme issue queue item, either from socialbase or socialblue. If any other issue trackers were used, include links to those too.

How to test

  • Wait for all the tests to be green again

Definition of done

Before merge

  • Code/peer review is completed
  • All commit messages are clear and clean. If applicable a rebase was performed
  • All automated tests are green
  • Functional/manual tests of the acceptance criteria are approved
  • All acceptance criteria were met
  • New features or changes to existing features are covered by tests, either unit (preferably) or behat
  • Update path is tested. New hook_updates should respect update order, right naming convention and consider hook_post_update code
  • Module can be safely uninstalled. Update/implement hook_uninstall and make sure that removed configuration or dependencies are removed/uninstalled
  • This pull request has all required labels (team/type/priority)
  • This pull request has a milestone
  • This pull request has an assignee (if applicable)
  • Any front end changes are tested on all major browsers
  • New UI elements, or changes on UI elements are approved by the design team
  • New features, or feature changes are approved by the product owner

After merge

  • Code is tested on all branches that it has been cherry-picked
  • Update hook number might need adjustment, make sure they have the correct order
  • The Drupal.org ticket(s) are updated according to this pull request status

Release notes

Internal. See problem/solution part

Change Record

Translations

We previously utilised a lot of clean-up steps to make sure our tests
didn't affect eachother. However we've since moved to a scenario where
both in the CI and locally a new database is loaded between scenarios,
this makes the clean-up no longer needed.

Additionally if the clean-up does run then this makes debugging a failed
test a lot more difficult because you can't inspect the state of the
platform at the time the test failed.

This commit ensures that all the clean-up that is provided by the
DrupalExtension by default is disabled. For this we have to disable
autoloading of sub-contexts (which we weren't using anyway) because the
search_api contained a `behat.inc` file that would load the
`RawDrupalContext` class which contained more cleaning.

There are DrupalExtension stories open to remove the cleaning from
`RawDrupalContext`, but we don't have time to wait for that.

We also add a step in our CI to dump the failed database to disk along
with the installation.sql file. This allows a developer to load the
failed database locally and see what was going wrong without having to
rerun the test.
In newly written tests we were already doin this manually in each
scenario. Between scenario's the user manager in the DrupalExtension was
not correctly keeping track of the logged in user status which could
prevent a new scenario from logging a user in.

With the removal of clean-up in scenario's we now need that logout step
literally everywhere, so it's easier to add it as an `@afterScenario`
and run it every time than to require developers to decide about 100s of
places whether it's needed.

See jhedstrom/drupalextension#641
These were used in the past to help set-up test automation on Travis but
they don't actually test Open Social. In any scenario that these tests
might fail other tests would fail too.

The tests are removed to reduce the number of parallel tests being
executed.
@robertragas robertragas added status: needs review This pull request is waiting for a requested review type: revert Reverts a previous PR or commit prio: high internal team: guardians labels Jul 22, 2023
@mergeable
Copy link

mergeable bot commented Jul 22, 2023

Thanks for contributing towards Open Social! A maintainer from the @goalgorilla/maintainers group might not review all changes from all teams/contributors. Please don't be discouraged if it takes a while. In the meantime, we have some automated checks running and it might be that you will see our comments with some tips or requests to speed up the review process. 😊

@robertragas robertragas changed the title Bugfix/port GitHub issue 3393 Internal: Backport AvoidCleanupTrait to 11.9.x Jul 22, 2023
@robertragas
Copy link
Contributor Author

Also tests failing here, likely due to a change in the test entitytrait validation thats inside 11.9.x but not main. Will take another look on monday.

It's about the validation of the created date that doesn't allow 01/01/01.

This commit allows taxonomy reference data to be provided as a comma
separated list of IDs or term names (mixing is allowed). This can now be
used in tables for e.g. groups, topics and events. By putting this in
the validate method we can use this everywhere.

This ensures we deduplicate the taxonomy title lookup so that we don't
break on a pre-fetched value. Since we now do it by field type we no
longer need to do it by field name.

We can also remove the topic clean-up since we load a new database
before every test. This allows us to remove the helper that looks-up a
topic type by name.
We can use our EntityTrait to allow any relative date for datetime
fields that we create using any of our newer content scaffolding test
steps.
These fields have a different field type and value structure than the
`datetime` fields that we already tackled in a previous commit, so we
need to add them separately.
@robertragas
Copy link
Contributor Author

Added 3 more cherry-picks for the failing tests

526a3e9
6fc3edd
4839b3a

@ribel ribel self-requested a review July 24, 2023 07:19
Copy link
Contributor

@ribel ribel left a comment

Choose a reason for hiding this comment

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

LGTM

@ribel ribel added this to the 11.9.8 milestone Jul 24, 2023
@robertragas robertragas merged commit 5a8192e into 11.9.x Jul 24, 2023
183 checks passed
@robertragas robertragas deleted the bugfix/port-github-issue-3393 branch July 24, 2023 07:31
@Kingdutch Kingdutch added type: repository Improvements to working with the repository (e.g. templates, README files, or workflows) and removed internal labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: high status: needs review This pull request is waiting for a requested review team: guardians type: repository Improvements to working with the repository (e.g. templates, README files, or workflows) type: revert Reverts a previous PR or commit
3 participants