Feature into system specs #8179

filipefurtad0 · 2021-09-13T22:11:00Z

What? Why?

Replaces #8103.
Enables #7929.

There are quite a few ways to approach this problem :-)

Similar to #8103 but moves all feature specs into /system as they are updated to pass. This is the opposite approach taken in #8103 in which the down side was that during the lifetime of this branch the updates made to specs were being missed and generating conflicts.

~~The approach chosen here should allow rebasing and not missing any updates to specs, introduced by other PRs.~~

We just is now only rename spec blocks, this sets up for the upcoming changes.

What should we test?

Green build.

Release notes

Changes feature specs into system specs

Changelog Category: Technical changes

Dependencies

Documentation updates

mkllnk

This is so much better, great! And the specs pass! 🎉

This is basically ready to go. There are just a few unnecessary changes. Otherwise I would have approved. 😄

mkllnk · 2021-09-22T06:07:45Z

spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb

+describe '
+    As an administrator
+    I want to create/update complex order cycles with a specific time
+', js: true do


These specs seem to be copied instead of moved.

mkllnk · 2021-09-22T06:09:02Z

spec/features/admin/order_cycles/complex_creating_specific_time_spec.rb

-describe '
-    As an administrator
-    I want to create/update complex order cycles with a specific time
-', js: true do


Ah, now I see. These two commits should have been one.

mkllnk · 2021-09-22T06:11:18Z

spec/features/admin/adjustments_spec.rb

-require "system_helper"
+require "spec_helper"


This commit is what we call a "detour". You first did one change and then you undo it later. It makes reviewing slightly more confusing. Maybe you want to keep this in a WIP branch because you expect to change all feature specs at some point but for the purpose of this pull request, it would be nicer to squash these changes into the first commit.

mkllnk · 2021-09-22T06:15:42Z

spec/features/admin/adjustments_spec.rb

-describe '
+feature '


This change doesn't need to be reverted. You can actually rename all feature to describe. It's the same. Someone just had the idea that feature sounds nicer and introduced it as an alias for describe.

I'm not so sure about this @mkllnk , lets take spec/features/admin/multilingual_spec.rb as an example:

with 'spec_helper' - using feature (L5) - using background (L11) - spec passes

with 'spec_helper' - using describe (L5) - using background (L11) - spec fails

with 'spec_helper' - using describe (L5) - using before (L11) - spec passes

It seems to make a difference.

mkllnk · 2021-09-22T06:17:11Z

spec/features/admin/bulk_order_management_spec.rb

-    describe "sorting of line items" do
+    feature "sorting of line items" do


This line actually never said feature. It was describe before. 😉

mkllnk · 2021-09-22T07:01:29Z

@filipefurtad0 Sometimes showing is much easier than explaining. I created a new branch to show you how I would organise the commits for this pull request:

master...mkllnk:feature-to-system-1-rebased

The first commit will be helpful for all future work on this. And then, after moving specs from feature to system, I simply run this:

git ls-files -- spec/system/ | xargs sed -i 's/spec_helper/system_helper/'

That makes it easy to migrate specs in chunks without reverting anything. I also removed a commented line and fixed up some whitespace. Feel free to reset your branch to mine. 😜

filipefurtad0 · 2021-09-23T15:46:21Z

This is reeeally useful for the non-dev 😄

filipefurtad0 · 2021-09-24T15:51:59Z

I think I'll need help here @mkllnk , I'm not sure how to proceed on resetting my branch onto yours:

I've tried merging your commits to my branch, but get conflicts:
Another approach would be to fork your branch, get it onto my local system and resetting my branch on top of yours, but this did not work:

I got the commit hash from here:

Not sure. Maybe I could revert that last commit in which I reverted describe -> feature. Let's have a quick call on it, if you fancy.

mkllnk · 2021-09-26T23:23:23Z

Git is very confusing to start with and then very powerful. This seems to work for me:

git checkout feature_into_system
git reset --hard 98c98e59e55b5bb08c48def9c235defd07a5e0af

# Then update your PR:
git push --force

filipefurtad0 · 2021-09-27T09:07:32Z

Git is very confusing to start with and then very powerful

Indeed! 👍

That hash seems to lead to a missing commit:

fatal: Could not parse object '98c98e59e55b5bb08c48def9c235defd07a5e0af'.

I think I know what happened @mkllnk I tried to revert my last commit and since yours was on top of it, it might have gone deleted. This branch is now as it was before, except your commit is missing. I think you'd need to commit again, sorry for that!

mkllnk · 2021-09-27T22:11:03Z

That hash seems to lead to a missing commit:

Ah, the problem is that the commit is in my repository but not in yours. You would have needed to fetch my branch from my repository first. But let's shortcut this. I'll just push my version.

The `feature` and `scenario` names are aliases only available in feature specs and not needed. It's confusing to have different names and we can easily move feature specs to system specs when using standard names.

mkllnk · 2021-09-27T22:42:40Z

There are now some spec failures. 😞

filipefurtad0 · 2021-09-28T09:13:29Z

spec/features/admin/adjustments_spec.rb

@@ -2,7 +2,7 @@

 require "spec_helper"

-feature '
+describe '


I think we can't have specs using spec_helper with describe, as they will be run as system specs, but will break because they are not calling the necessary helper (system_helper).

I'm noticing this as the build is failing here, on background:

https://github.com/openfoodfoundation/openfoodnetwork/pull/8179/files#diff-0f3532dd56c5ea48814fa945b58cd9a5851a2ddc631cd91020d85e25d783f1e8R319

Which passes on feature specs but breaks system specs -> replacing with before fixes it.

This seems to prevent the build from running:

Replaces 'background' with 'before' Replaces 'background' with 'before' in other files Fixes typo on products_spec.rb

mkllnk

Great, everything passes. I'm very happy with this pull request as is but the description doesn't fit any more. This is now only renaming spec blocks. There's no moving of specs left. That's okay. If we get this low-risk PR through, it will avoid a lot of merge conflicts in the future and we can do it step by step.

mkllnk · 2021-09-29T01:33:42Z

spec/features/admin/multilingual_spec.rb

-  background do
+  before do


Great find! I forgot about this syntax, another feature-spec specific naming that is not necessary.

filipefurtad0 · 2021-09-29T15:22:58Z

Updated the description. Once this gets merged I'll move the files previously moved, and make the changes as we go 👍

andrewpbrett

Nice work @filipefurtad0 👍

filipefurtad0 force-pushed the feature_into_system branch 2 times, most recently from 125cf57 to 9880c25 Compare September 20, 2021 13:03

filipefurtad0 closed this Sep 20, 2021

filipefurtad0 force-pushed the feature_into_system branch from 9880c25 to 02a10ed Compare September 20, 2021 13:13

filipefurtad0 reopened this Sep 20, 2021

filipefurtad0 mentioned this pull request Sep 20, 2021

Feature into system specs #8103

Closed

mkllnk requested changes Sep 22, 2021

View reviewed changes

filipefurtad0 force-pushed the feature_into_system branch from f77add9 to 628fac9 Compare September 27, 2021 09:03

mkllnk force-pushed the feature_into_system branch from 628fac9 to 98c98e5 Compare September 27, 2021 22:14

Standardise spec block naming

6f694e2

The `feature` and `scenario` names are aliases only available in feature specs and not needed. It's confusing to have different names and we can easily move feature specs to system specs when using standard names.

mkllnk force-pushed the feature_into_system branch from 98c98e5 to a043fe6 Compare September 27, 2021 22:26

filipefurtad0 commented Sep 28, 2021

View reviewed changes

Replaces 'background' with 'before' (squashing)

81890a4

Replaces 'background' with 'before' Replaces 'background' with 'before' in other files Fixes typo on products_spec.rb

filipefurtad0 force-pushed the feature_into_system branch from 5a8f9cd to 81890a4 Compare September 28, 2021 10:33

mkllnk approved these changes Sep 29, 2021

View reviewed changes

andrewpbrett approved these changes Oct 5, 2021

View reviewed changes

andrewpbrett merged commit ec0bb63 into openfoodfoundation:master Oct 5, 2021

andrewpbrett mentioned this pull request Oct 5, 2021

Switches assertion order #8267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature into system specs #8179

Feature into system specs #8179

filipefurtad0 commented Sep 13, 2021 •

edited by mkllnk

Loading

mkllnk left a comment

mkllnk Sep 22, 2021

mkllnk Sep 22, 2021

mkllnk Sep 22, 2021

mkllnk Sep 22, 2021

filipefurtad0 Sep 28, 2021 •

edited

Loading

mkllnk Sep 22, 2021

mkllnk commented Sep 22, 2021 •

edited

Loading

filipefurtad0 commented Sep 23, 2021 •

edited

Loading

filipefurtad0 commented Sep 24, 2021 •

edited

Loading

mkllnk commented Sep 26, 2021

filipefurtad0 commented Sep 27, 2021

mkllnk commented Sep 27, 2021

mkllnk commented Sep 27, 2021

filipefurtad0 Sep 28, 2021

filipefurtad0 Sep 28, 2021 •

edited

Loading

mkllnk left a comment

mkllnk Sep 29, 2021

filipefurtad0 commented Sep 29, 2021

andrewpbrett left a comment

		describe "sorting of line items" do
		feature "sorting of line items" do

Feature into system specs #8179

Feature into system specs #8179

Conversation

filipefurtad0 commented Sep 13, 2021 • edited by mkllnk Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Sep 22, 2021

Choose a reason for hiding this comment

mkllnk Sep 22, 2021

Choose a reason for hiding this comment

mkllnk Sep 22, 2021

Choose a reason for hiding this comment

mkllnk Sep 22, 2021

Choose a reason for hiding this comment

filipefurtad0 Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

mkllnk Sep 22, 2021

Choose a reason for hiding this comment

mkllnk commented Sep 22, 2021 • edited Loading

filipefurtad0 commented Sep 23, 2021 • edited Loading

filipefurtad0 commented Sep 24, 2021 • edited Loading

mkllnk commented Sep 26, 2021

filipefurtad0 commented Sep 27, 2021

mkllnk commented Sep 27, 2021

mkllnk commented Sep 27, 2021

filipefurtad0 Sep 28, 2021

Choose a reason for hiding this comment

filipefurtad0 Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Sep 29, 2021

Choose a reason for hiding this comment

filipefurtad0 commented Sep 29, 2021

andrewpbrett left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Sep 13, 2021 •

edited by mkllnk

Loading

filipefurtad0 Sep 28, 2021 •

edited

Loading

mkllnk commented Sep 22, 2021 •

edited

Loading

filipefurtad0 commented Sep 23, 2021 •

edited

Loading

filipefurtad0 commented Sep 24, 2021 •

edited

Loading

filipefurtad0 Sep 28, 2021 •

edited

Loading