Improve tests stability #296

Ptrboro · 2023-10-22T13:17:57Z

✍️ Description

This PR addresses some inconsistencies in our test suite to enhance its stability and the overall developer experience.

Here's a summary of the improvements made:

Changed the setup function in test_helper.rb to ensure it is called before the setup block in individual test files.
Resolved issues of Organizations generated with factories having duplicate slugs.
Clarified interconnected associations in factories to ensure consistency:

match = create(:match)
match.organization == match.pet.organization # => now true

Resolved issues of Users generated with factories having duplicate emails.
Streamlined the behavior of factories with ActsAsTenant:

user = create(:user)
user.organization == ActsAsTenant.test_tenant # => true
Organization.count # => 1 (was 2 before the fix)

Reconfigured ActsAsTenant for tests according to the docs

Ptrboro · 2023-10-22T13:24:54Z

app/controllers/organizations/invitations_controller.rb

+  include OrganizationScopable
+  # TODO: remove next line once we fix links in the invitation emails
+  skip_before_action :set_tenant, except: [:new, :create]


After tweaking the ActsAsTenant setup in our tests, one of the tests started failing. There was an actual bug and StaffAccount kept linking itself to the organization with an id of 1. Including OrganizationScopable fixes that up, but we should probably drop the default value from the staff_account.organization_id column so we don't miss similar issues down the road.

Ptrboro · 2023-10-22T13:27:23Z

test/factories.rb

-    trait :application_awaiting_review do
-      adopter_account
-
-      after :create do |user|
-        create :adopter_application, adopter_account: user.adopter_account, status: 0
-        user.adopter_account.adopter_profile = create(:adopter_profile)
-      end


application_awaiting_review trait was creating 4 different organizations under the hood. It was hard to fix so I decided it's better to remove it and build these associations in the test itself (test/integration/adoptable_pet_show_test.rb)

Ptrboro · 2023-10-22T13:39:16Z

test/test_helper.rb

@@ -26,12 +26,12 @@ def set_organization(organization)
    Rails.application.routes.default_url_options[:script_name] = "/#{organization.slug}"
  end

-  def setup
-    ActsAsTenant.current_tenant = create(:organization, slug: "test")
+  setup do


The issue was that def setup is called after the setup blocks in individual tests:

# test/test_helper.rb def setup puts "I'm in test_helper.rb!" end # test/some_dummy_test.rb setup do puts "I'm in dummy_test.rb!" end # > I'm in dummy_test.rb! # > I'm in test_helper.rb!

By converting it to setup do; ... end we fix the order issue :)

# test/test_helper.rb setup do puts "I'm in test_helper.rb!" end # test/some_dummy_test.rb setup do puts "I'm in dummy_test.rb!" end # > I'm in test_helper.rb! # > I'm in dummy_test.rb!

This is important because we create an organization in this setup and assign it as current test tenant.

kasugaijin · 2023-10-23T01:44:20Z

Very cool @Ptrboro thank you for looking into this. I need a bit more time to digest what's going on so unless someone beats me to it I review this week.

kasugaijin · 2023-10-23T21:11:11Z

test/factories.rb

    end
  end

  factory :organization do
    name { Faker::Company.name }
-    slug { Faker::Internet.domain_word }
-    profile { build(:organization_profile) }
+    sequence(:slug) { |n| Faker::Internet.domain_word + n.to_s }


so we append a number here just to ensure it's unique? i.e., in the slim case Faker creates the same word twice.

Exactly :) One of the tests I was debugging was sometimes failing because of this (but I can't remember now which one).

kasugaijin

Looks great to me! For the most part you're making the tenant instance available and explicitly defining which org to associate to in the test data used by Faker. It makes sense to me and I cannot see anything I would change. This looks like a far more robust approach.

* Fix global tests setup * Ensure organization slugs are uniq in tests * Fix factories so they use the same organization for associations * Ensure user emails are uniq in tests * Use current_tenant in factories * Use ActsAsTenant.test_tenant in tests * run standard:fix

* Update with partial * Update with attempt with nice_partials gem * Syntax cleanup * Added new nice_partails folder * Syntax cleanup * Update with path params value changed * Update Front-end spacing clean up * Added new partials and Table to DB * Update Changed the task list to now be a modal * Post merge commit * Add Applications list to Pet show (#288) * Update sidebar styling to avoid spilling to tab links Co-authored-by: FionaDL fionadlapham@gmail.com * Fix applicant name on applications page Co-authored-by: FionaDL fionadlapham@gmail.com * add applications partial and test Co-authored-by: FionaDL fionadlapham@gmail.com * add tests for application tab Co-authored-by: FionaDL fionadlapham@gmail.com * Clear default_url_options[:script_name] after each test (#287) * Added edit partial * 281: Pets Show Overview Tab make Edit and Delete buttons more visible (#286) * Replace dropdown button with edit button, add delete button beneath summary card * Modify delete button styling * docs: add jadekstewart3 as a contributor for code (#291) * docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> * docs: add edwinthinks as a contributor for code (#292) * docs: update README.md [skip ci] * docs: update README.md [skip ci] * docs: update .all-contributorsrc [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Ben <95949082+kasugaijin@users.noreply.github.com> * Org dashboard (#290) * Add instance variables that will be used in the dashboard view * Add template html and begin inital formatting to utilize instance variables scoped to organization * remove img tag for non existant avatar * Merge conflict updated * Merge conflict updated * Add column 2 org profiles (#289) * Create migration file to add about us to organization profiles table, and add column to seed files * add about us field for organization_profiles * add validation for about us * add validation for about us * standard * Remove validation that we didnt need * remove null false from migration file and migrate * standard rb * add validation for about us * Add back in accidentially removed validation * Migrate updated migration file * Add null false back into correct place and migrate * remove not null comment * standard rb * merge main * Remove default values from foreign keys (#298) * Improve UI of Accept Invitation Form (#300) * Improve tests stability (#296) * Fix global tests setup * Ensure organization slugs are uniq in tests * Fix factories so they use the same organization for associations * Ensure user emails are uniq in tests * Use current_tenant in factories * Use ActsAsTenant.test_tenant in tests * run standard:fix * fix all contributors table (#293) * 268 part 1: staff can upload profile pic (#276) * Add model changes and tests * Add form field, and changes into the controller * change naming from Picture to Avatar * Run standard fix * Move avatarable tests to shared module * Add uniq index to organization slug (#302) * Merge conflict updated * Update with working but ugly crud features * Syntax error cleanup * Removed binding * Added partial files and corrected before acgtion * Remove 'overview' from the default active tab options * Update Frontend form still nested * Linter error cleanup * Merge conflict resolved * Pet controller refactor * Update with code clean up * Pet controller syntax cleanup * Removed partials * Cleanup Syntax * Test file merge conflict solution * Syntax cleanup * Code refactor * Update spacing in file clean up * Updated frontend with turbo delete button * Update with Features rendering correctly but code is sloppy * Standard error fix * Update spacing and code clean up * Removed unneeded CSS file * Removed more unneeded css * Removed hardcoded org * Added turbo_stream file and last commit before migration that moves away from Tasks * Update with skipped unit tests * Syntax cleanup * Update to spacing * Update spacing cleanup * Update spacing cleanup part 2 * Update spacing and syntax cleanup part 3 * Update styling changes to tasks * Added model tests * Update with passing tests * Update syntax cleanup * Update syntax spacing * Update syntax spacing * Update syntax spacing * Update with Frontend cleanup * Testing all passing * Spacing cleanup * Update cleanup * Update spacing cleanup * Added null:false to tasks name * Added migration file * Added updated test --------- Co-authored-by: aisayo <aysan@ombulabs.com> Co-authored-by: Piotr Borowiec <p.borowiec01@gmail.com> Co-authored-by: MooseCowBear <82609260+MooseCowBear@users.noreply.github.com> Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Ben <95949082+kasugaijin@users.noreply.github.com> Co-authored-by: Jade Stewart <114014697+jadekstewart3@users.noreply.github.com> Co-authored-by: Marlena Borowiec <96994176+marlena-b@users.noreply.github.com> Co-authored-by: Yuri Pains <yuricarvalhop@gmail.com>

Ptrboro commented Oct 22, 2023

View reviewed changes

Ptrboro force-pushed the tests-improvements branch from 46a3406 to 9b6aaa6 Compare October 22, 2023 13:29

Ptrboro commented Oct 22, 2023

View reviewed changes

Ptrboro mentioned this pull request Oct 22, 2023

Remove defaults from columns with foreign keys #297

Closed

3 tasks

Ptrboro requested review from edwinthinks, nsiwnf and kasugaijin October 22, 2023 13:53

Ptrboro marked this pull request as ready for review October 22, 2023 13:57

Ptrboro changed the title ~~Tests improvements~~ Improve tests stability Oct 22, 2023

marlena-b mentioned this pull request Oct 23, 2023

Remove default values from foreign keys #298

Merged

kasugaijin reviewed Oct 23, 2023

View reviewed changes

kasugaijin approved these changes Oct 23, 2023

View reviewed changes

Ptrboro added 7 commits October 24, 2023 15:58

Fix global tests setup

523c817

Ensure organization slugs are uniq in tests

5dd520c

Fix factories so they use the same organization for associations

5708540

Ensure user emails are uniq in tests

fa6668d

Use current_tenant in factories

d8e6b8f

Use ActsAsTenant.test_tenant in tests

43802e4

run standard:fix

8ef216c

Ptrboro force-pushed the tests-improvements branch from 9b6aaa6 to 8ef216c Compare October 24, 2023 13:59

Ptrboro merged commit 779d5a2 into main Oct 24, 2023
3 checks passed

Ptrboro deleted the tests-improvements branch October 24, 2023 14:03

mononoken mentioned this pull request Feb 8, 2024

Fix test tenant leakage #465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tests stability #296

Improve tests stability #296

Ptrboro commented Oct 22, 2023 •

edited

Ptrboro Oct 22, 2023 •

edited

Ptrboro Oct 22, 2023 •

edited

Ptrboro Oct 22, 2023

kasugaijin commented Oct 23, 2023

kasugaijin Oct 23, 2023

Ptrboro Oct 24, 2023

kasugaijin left a comment

Improve tests stability #296

Improve tests stability #296

Conversation

Ptrboro commented Oct 22, 2023 • edited

✍️ Description

Ptrboro Oct 22, 2023 • edited

Choose a reason for hiding this comment

Ptrboro Oct 22, 2023 • edited

Choose a reason for hiding this comment

Ptrboro Oct 22, 2023

Choose a reason for hiding this comment

kasugaijin commented Oct 23, 2023

kasugaijin Oct 23, 2023

Choose a reason for hiding this comment

Ptrboro Oct 24, 2023

Choose a reason for hiding this comment

kasugaijin left a comment

Choose a reason for hiding this comment

Ptrboro commented Oct 22, 2023 •

edited

Ptrboro Oct 22, 2023 •

edited

Ptrboro Oct 22, 2023 •

edited