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.

Sign up for GitHub

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

Jump to bottom

Internal: Disable clean-up in Behat tests #3393

Merged
merged 3 commits into from May 4, 2023
Merged

Internal: Disable clean-up in Behat tests #3393

merged 3 commits into from May 4, 2023

Conversation

Kingdutch
Copy link
Member

Problem / Solution

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.

Issue tracker

Internal no issue

How to test

  • Create a test failure locally
  • Login to the platform as admin
  • You should be able to see everything up to the test step failure

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

@Kingdutch
Disable clean-up in Behat tests
e9c6e4d 
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.
@mergeable
Copy link

mergeable bot commented May 4, 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. 😊

@Kingdutch Kingdutch changed the title Disable clean-up in Behat tests Internal: Disable clean-up in Behat tests May 4, 2023
@mergeable
Copy link

mergeable bot commented May 4, 2023

❌ Error Occurred while executing an Action

If you believe this is an unexpected error, please report it on our issue tracker: https://github.com/mergeability/mergeable/issues/new

Error Details

HttpError: Invalid request.

No subschema in "anyOf" matched.
For 'anyOf/0', {"labels"=>{"labels"=>["blocked: mergeable"]}} is not an array.
For 'anyOf/1', {"labels"=>{"labels"=>["blocked: mergeable"]}} is not an array.
For 'properties/labels', {"labels"=>["blocked: mergeable"]} is not an array.

@mergeable
Copy link

mergeable bot commented May 4, 2023

❌ Error Occurred while executing an Action

If you believe this is an unexpected error, please report it on our issue tracker: https://github.com/mergeability/mergeable/issues/new

Error Details

HttpError: Invalid request.

No subschema in "anyOf" matched.
For 'anyOf/0', {"labels"=>{"labels"=>[]}} is not an array.
For 'anyOf/1', {"labels"=>{"labels"=>[]}} is not an array.
For 'properties/labels', {"labels"=>[]} is not an array.

@Kingdutch
Logout after each Behat scenario
75d4ee9 
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
@ronaldtebrake ronaldtebrake added prio: high internal type: repository Improvements to working with the repository (e.g. templates, README files, or workflows) labels May 4, 2023
@ronaldtebrake
Copy link
Contributor

Nice, that could be super helpful, cheers @Kingdutch 🚀

@Kingdutch
Remove dev-helpers tests
31c57dd 
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.
@Kingdutch Kingdutch merged commit dd55057 into main May 4, 2023
7 checks passed
@Kingdutch Kingdutch deleted the internal/failed-test-database branch May 4, 2023 16:25
@robertragas robertragas mentioned this pull request Jul 22, 2023
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldtebrake ronaldtebrake Awaiting requested review from ronaldtebrake

Assignees
No one assigned
Labels
prio: high type: repository Improvements to working with the repository (e.g. templates, README files, or workflows)
Milestone
No milestone
2 participants
@Kingdutch @ronaldtebrake