Uk/adapt tests #1521

jeronimo · 2017-04-04T16:26:32Z

No description provided.

enricostano · 2017-04-06T15:56:21Z

@jeronimo did you give a look at #1357 ?

jeronimo · 2017-04-06T22:01:02Z

I haven't. Thanks for pointing it out.
Potentially #1357 hardcoding raises issues like in https://community.openfoodnetwork.org/t/newcomers-issues/876 3) point.
Meaning that ./spec/features/admin/bulk_order_management_spec.rb:435 test fails with the code in ##1357 when Australia moves to next day and local machine stays in the current (at least for me).

I would suggest to have as little hardcoding as possible and to have all settings in one file (in our case in application.yml). And eventually I think we will have to write tests where we will test multiple currencies.

enricostano · 2017-04-07T08:49:10Z

Australia moves to next day and local machine stays in the current (at least for me)

I think that we should all use the same configuration for tests and not relying on application.yml. In theory we all should run the tests in the same TZ, locale, etc.

The reasoning behind that is that we should be able to compare tests results. Right now for instance we have differences between Travis CI and our local machines, and some time tests fail in Travis but not locally.

IMHO same environment for everybody + activating random order execution for tests should be the way to go.

More thoughts?

jeronimo · 2017-04-07T09:11:25Z

The problem for me is that #1357 approach fails one test /spec/features/admin/bulk_order_management_spec.rb:435 with that code when I run test after 4pm on my local machine with UTC time. That's why I am a little bit against it :) Can we fix that test? I couldn't find quick fix for it

I wouldn't mind having one currency/country/state/timezone setting in tests if everyone knows about it. Though it won't be good if some developers will hardcode $ by accident instead of £ or € and we won't be able to check that and it will go through builds to UK/EU users.
Travis and other CI can have hardcoded Australian settings but for myself I would like to run tests with £ or € in my local machine before making pull request.

enricostano · 2017-04-07T09:31:38Z

I wouldn't mind having one currency/country/state/timezone setting in tests if everyone knows about it. Though it won't be good if some developers will hardcode $ by accident instead of £ or € and we won't be able to check that and it will go through builds to UK/EU users.

we can enforce this automating bad practices checks in CI

Travis and other CI can have hardcoded Australian settings but for myself I would like to run tests with £ or € in my local machine before making pull request.

I think we should have some specific test only testing that all the supported localizations works. But the rest of the tests should be in only one locale / currency. In that way you can ensure that everythings works OK in your currency and you don't break others.

What do you think?

enricostano · 2017-04-07T09:41:53Z

This article can shade some light on how system time, selected time and application time is managed in Rails: https://www.michaelcho.me/article/understanding-time-datetime-and-timezones-in-rails

jeronimo · 2017-04-07T09:42:14Z

#1357 approach has failing test for me.
#1521 passes by. And it doesn't need to add bad practices checks nor adding additional tests for localisation works. At least for now.

Anyhow, it is just my opinion. Maybe others and elders :) could give their opinion :)

daniellemoorhead · 2017-04-21T04:03:18Z

@Matt-Yorkley @jeronimo @enricostano can the three of you please connect and discuss what should happen with these two PRs (this one and #1357), and then let us know what you decide and we'll move forward with that. We're not sure what to do in the meantime so will await your instructions and not touch either.

mkllnk

This is good work. My only concern is the readability of the specs. Let me know your thoughts.

mkllnk · 2017-05-03T04:37:23Z

Gemfile

@@ -115,7 +115,7 @@ end

 group :test do
  gem 'webmock'
-
+  gem 'capybara-screenshot'


Interesting gem. But it's completely unrelated to the issue. Can you open a separate pull request for this?

I was trying to squeeze this gem in master for a while now :) Will create new PR!

mkllnk · 2017-05-03T04:55:27Z

spec/features/consumer/shopping/cart_spec.rb

-        expect(page).to have_selector '.cart-item-total',         text: '$8.24'
-        expect(page).to have_selector '.order-total.item-total',  text: '$8.24'
-        expect(page).to have_selector '.order-total.grand-total', text: '$8.24'
+        expect(page).to have_selector '.cart-item-price',         text: "#{@currency_symbol}1.03"


There are heaps of these checks. And they all become less readable with this change. Have you looked at app/helpers/spree_currency_helper.rb? The idea is good, but the method name is too long. What do you think about a helper just called money or display_money?

This line would read like:

expect(page).to have_selector '.cart-item-price', text: display_money(1.03)

If a Money object would be converted to a string automatically, we could have a helper called money and it would be even shorter.

I agree... Fast and ugly workaround to skip Capybara.default_max_wait_time = 30 when tests fail.

Unfortunately I couldn't go with Spree::Money.new(amount).to_s helper, because it works as full helper, meaning that it will return "$1,223.00" when 1223 is provided and in the code there and it seems that OFN codebase doesn't use spree_number_to_currency helper everywhere.

I don't understand the connection to the wait time here.

If we don't use that helper, what do we use instead? We should probably unify how we display prices and have a helper for that.

We should use that helper. And probably will have to write one for Angular front-end to unify (if there is none). Basically will have to go through each failing test and check what is happening with each method. Just please give me more time for that :)

As for money helper in specs I would suggest it is a bit too short and doesn't say what it does, just something with money, it is like thing(1.03) and you still have to look what it does in the code (which is not a bad thing). display_money would mean that it gives output, right? Maybe text: with_currency(1.03) would be more meaningful? Apologies if github's PR comments is not a good place for discussions like this.

with_currency sound good.

mkllnk · 2017-05-03T04:58:54Z

spec/spec_helper.rb

@@ -93,6 +93,9 @@
  # Ensure we start with consistent config settings
  config.before(:each) { Spree::Config.products_require_tax_category = false }

+  # Set currency symbol from config for tests
+  config.before(:all) { @currency_symbol = ::Money::Currency.new(Spree::Config[:currency]).symbol }


Good thought, but I don't think it's a good practice to set global variables. A helper would enforce that it's readonly. It could also be shared between the app and the tests. Last but not least, a helper can be tested itself.

mkllnk · 2017-05-03T05:09:57Z

spec/features/admin/order_cycles_spec.rb

-
-    page.should have_selector "input[value='2040-11-06 06:00:00 +1100']"
-    page.should have_selector "input[value='2040-11-13 17:00:00 +1100']"
+    page.should have_selector "input[value='#{Time.zone.local(2040, 11, 06, 06, 00, 00)}']"


Cool. Since these are repeating a few times, it would be even better to define them in the beginning:

let(:meaningful_time) { Time.zone.local(2040, 11, 06, 06, 00, 00) }

What do you think?

mkllnk · 2017-05-03T05:11:15Z

Gemfile.lock

@@ -754,4 +754,4 @@ RUBY VERSION
   ruby 2.1.5p273

 BUNDLED WITH
-   1.14.6
+   1.14.3


If you squash that into the commit that changed this line in the first place, then you remove this little detour. But I actually don't mind the bundler version here when other parts of the file change as well.

Matt-Yorkley · 2017-05-03T15:24:33Z

@jeronimo Just a quick note to say we already have capybara screenshot functionality, you can use: save_and_open_screenshot.

jeronimo · 2017-05-04T17:16:48Z

@mkllnk and @RohanM please review again when you'll have time! :)

mkllnk

Much better. I'm not sure about the Spree namespace of the helper, since we are not overriding a Spree functionality. But I don't have a strong opinion on that either.

Thank you!

enricostano · 2017-05-08T09:39:44Z

So, we're going back to dynamic specs? I'm really concerned about this.

jeronimo · 2017-05-08T09:48:37Z

UK CI will test £ and UTC, French CI will test € and UTC +2. Australia will test $ with UTC +9.
Developers running tests in their own machines will test the same.

OR someone needs to write tests now to cover all cases where currency or timezone can change.

enricostano · 2017-05-09T13:37:28Z

We have only 1 CI, right? We should at least.

OR someone needs to write tests now to cover all cases where currency or timezone can change.

Yes, we should have a small suite checking that.

enricostano · 2017-05-09T13:45:15Z

But again, if I'm the only one concerned... go ahead.

mkllnk · 2017-05-12T02:23:10Z

So, we're going back to dynamic specs? I'm really concerned about this.

Are you concerned about this as the only approach or do you think that it is a bad idea to have flexible specs?

My thought was that this enables some devs to test certain cases on their machine for now. But I don't think that it solves the problem that the application.yml influences the specs. That still needs to be solved. Specs should run with a default locale. Once we enable users to switch locales, we need some specs to test that as well. Some of them would be explicit like "when uk, expect pound" and others could be flexible like "when aus, expect money_with_currency(5)" and "when uk, expect money_with_currency(5)". These could use shared examples. It depends on the logic you are testing.

I agree that not all specs should be flexible. Otherwise we wouldn't pick up certain errors in which the app and the specs deal with dollars while we want to test pounds.

enricostano · 2017-05-12T07:54:42Z

I don't think that it solves the problem that the application.yml influences the specs

This is my main concern. The specs should be the same in CI as in every development environment. If not we'll start divergencies and can be difficult to spot issues and help others.

Once we enable users to switch locales, we need some specs to test that as well. Some of them would be explicit like "when uk, expect pound" and others could be flexible like "when aus, expect money_with_currency(5)" and "when uk, expect money_with_currency(5)". These could use shared examples. It depends on the logic you are testing.

Absolutely, yes. I'm not against specs helpers, shared examples, etc. My concern is only related to application.yml and other single OFN instance configuration affecting specs.

mkllnk · 2017-05-12T08:01:34Z

Good, we are on the same page. Thank you for the input. This pull request was maybe a bit more than we would like to have long term. Let's focus on becoming independent of application.yml.

daniellemoorhead assigned jeronimo Apr 21, 2017

daniellemoorhead added aus review labels Apr 21, 2017

daniellemoorhead requested a review from RohanM April 21, 2017 04:00

daniellemoorhead added blocked and removed aus review labels Apr 21, 2017

daniellemoorhead mentioned this pull request Apr 21, 2017

Set default currency for tests #1357

Closed

Em-AK mentioned this pull request Apr 21, 2017

Always use english locale in test environment #1505

Merged

daniellemoorhead removed the blocked label May 3, 2017

daniellemoorhead added this to the Next small release (1.8.11 or 1.9.1) milestone May 3, 2017

mkllnk requested changes May 3, 2017

View reviewed changes

jeronimo added 6 commits May 3, 2017 11:25

Add capybara-screenshot gem for integration tests debugging

41e9176

Use currency symbol from config in tests

dc69c6e

Use local time zone in tests instead of hardcoded

7c7933f

Restore bundler version

1e6f4aa

Use more simple getter to retrieve currency symbol

a97bcf7

Refactor time and currency symbol getters

e63f1c2

Implement with_currency rspec helper for money amounts

f25e3bc

mkllnk approved these changes May 5, 2017

View reviewed changes

RohanM merged commit f25e3bc into openfoodfoundation:master May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uk/adapt tests #1521

Uk/adapt tests #1521

jeronimo commented Apr 4, 2017

enricostano commented Apr 6, 2017 •

edited

jeronimo commented Apr 6, 2017 •

edited

enricostano commented Apr 7, 2017

jeronimo commented Apr 7, 2017

enricostano commented Apr 7, 2017

enricostano commented Apr 7, 2017

jeronimo commented Apr 7, 2017

daniellemoorhead commented Apr 21, 2017

mkllnk left a comment

mkllnk May 3, 2017

jeronimo May 3, 2017

mkllnk May 3, 2017

jeronimo May 3, 2017

mkllnk May 3, 2017

jeronimo May 4, 2017

mkllnk May 4, 2017

mkllnk May 3, 2017

mkllnk May 3, 2017

mkllnk May 3, 2017

Matt-Yorkley commented May 3, 2017 •

edited

jeronimo commented May 4, 2017

mkllnk left a comment

enricostano commented May 8, 2017

jeronimo commented May 8, 2017

enricostano commented May 9, 2017 •

edited

enricostano commented May 9, 2017

mkllnk commented May 12, 2017

enricostano commented May 12, 2017

mkllnk commented May 12, 2017

Uk/adapt tests #1521

Uk/adapt tests #1521

Conversation

jeronimo commented Apr 4, 2017

enricostano commented Apr 6, 2017 • edited

jeronimo commented Apr 6, 2017 • edited

enricostano commented Apr 7, 2017

jeronimo commented Apr 7, 2017

enricostano commented Apr 7, 2017

enricostano commented Apr 7, 2017

jeronimo commented Apr 7, 2017

daniellemoorhead commented Apr 21, 2017

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Yorkley commented May 3, 2017 • edited

jeronimo commented May 4, 2017

mkllnk left a comment

Choose a reason for hiding this comment

enricostano commented May 8, 2017

jeronimo commented May 8, 2017

enricostano commented May 9, 2017 • edited

enricostano commented May 9, 2017

mkllnk commented May 12, 2017

enricostano commented May 12, 2017

mkllnk commented May 12, 2017

enricostano commented Apr 6, 2017 •

edited

jeronimo commented Apr 6, 2017 •

edited

Matt-Yorkley commented May 3, 2017 •

edited

enricostano commented May 9, 2017 •

edited