Improves test coverage on Orders and Distributors report #12790

filipefurtad0 · 2024-08-19T22:09:32Z

Sets up an enterprise user instead of an admin user

What? Why?

Relates to [Test automation] Reports II #12818

Focuses on having a simple, dedicated system spec for Orders and Distributors report:

Checks that given enterprise user sees only their line items
Checks that datetime-picker works as expected
Checks admin sees all results. We've been experiencing issues around filters so a test was added to cover the distributor filter

It moves helper methods into its dedicated file, which affects other reports, so some changes were done there as well.

What should we test?

green build

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

filipefurtad0 · 2024-08-27T01:50:40Z

I'm a bit surprised about the failing test here. We have something very similar (actually, the same code) on ~/openfoodnetwork/spec/system/admin/reports/sales_tax/sales_tax_totals_by_order_spec.rb which fails locally with a timeout, but not on our build.

I suspect this might related to the fact that hitting the "Download report" opens a new window on the browser, rather than downloading the file to the local system...

filipefurtad0 · 2024-08-29T17:37:18Z

Ok, so at least there is some coherence now: both tests on PDFs fail. For some reason, this test has started to fail consistently. I'll remove the PDF assertion on it as well, as it is failing locally as well:

          downloads the file (PASS)
      behaves like reports generated as
        Spreadsheet
          downloads the file (PASS)
      behaves like reports generated as
        PDF
          downloads the file (FAILED - 1)

mkllnk

Good work!

I still think that we should move the data tests out of the system specs and have only one smoke test per report in system specs.

mkllnk · 2024-09-03T02:05:53Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+        Spree::LineItem.where(
+          order_id: ready_to_ship_order1.id # Total rows should equal nr. of line items, per order
+        ).count


I think that it would be more robust to replace this with the literal number.

mkllnk · 2024-09-03T02:12:00Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+        visit admin_reports_path
+        click_link "Orders And Distributors"


This is loading two pages when we need only the last one. It's good to go through the journey from reports path clicking on the report once, to proof that you can get to the report that way. But here you are doing it for several examples when you could visit the report directly.

Good point 👌

mkllnk · 2024-09-03T02:12:36Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+        rows = find("table.report__table").all("thead tr")
+        table_headers = rows.map { |r| r.all("th").map { |c| c.text.strip } }


We don't have a helper for this?

Hum, true this is used throughout report specs, although with slight variations. Would be great to unify this in a helper.

mkllnk · 2024-09-03T02:13:58Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+        it_behaves_like "reports generated as", "CSV", "csv"
+        it_behaves_like "reports generated as", "Spreadsheet", "xlsx"


Could it be that, depending on the version of Chrome, the PDF is opened in the browser instead of downloaded? That would explain the error, seeing a timeout because the file is never saved, only displayed.

PDF is opened in the browser instead of downloaded

I'm positive, that this is indeed the case - I've verified this in headless=false mode. But I have not found a way to download the file...

filipefurtad0 · 2024-09-03T04:05:29Z

I still think that we should move the data tests out of the system specs and have only one smoke test per report in system specs.

Ok, I did not realize that you literally meant only one smoke test 😅

I'll keep this in mind for the upcoming specs.

In the meantime, I'll mode this back to In Progress to address your comments. Thanks!

dacook

Looks good, but I think there's a couple of things we could improve before merging.

As an aside: I agree with Maikel that testing data can be done in unit tests which would be more efficient, not to mention easier. Based on your experience with coding now, would you be interested to try that next time? I'd be happy to give some pointers.
Here's the first random example I can find, which shows how the parameters are specified, and the results are checked: https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/lib/reports/bulk_coop_report_spec.rb#L62

dacook · 2024-09-04T04:11:55Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+            # for one distributor
+            select2_select distributor.name, from: "q_distributor_id_in"
+            run_report
+            expect(page).to have_content(distributor.name), count: 15


This doesn't seem to be checking that the other distributor was filtered out.

Awesome... Very sharp eye 🔍 The count: argument must within the brackets - if we use brackets at all! I've learned this yesterday the hard way, this was not very obvious to me. Thank for pointing it out.

dacook · 2024-09-04T04:13:05Z

spec/system/admin/reports/orders_and_fulfillment_spec.rb

              rows = find("table.report__table").all("tbody tr")
              table = rows.map { |r| r.all("td").map { |c| c.text.strip } }


Looks like we can remove this as it's now in the helper method.

Suggested change

rows = find("table.report__table").all("tbody tr")

table = rows.map { |r| r.all("td").map { |c| c.text.strip } }

dacook

Looks fine to me, but I have a question about the new pending spec:

dacook · 2024-09-08T23:59:02Z

spec/system/admin/reports/orders_and_fulfillment_spec.rb

+            xit "aggregates results per variant" do
+              pending '#9678'
+              expect(all('table.report__table tbody tr').count).to eq(4)
+              # 1 row per variant = 2 rows
+              # 2 TOTAL rows
+              # 4 rows total
+
+              expect(table_headers[0]).to eq(
+                ["Supplier Name", "Baked Beans", "1g Small, S",
+                 "Distributor Name", "7", "10.0", "70.0", "UPS Ground"]
+              )
+              expect(table_headers[1]).to eq(
+                ["", "", "", "TOTAL", "7", "", "70.0", ""]
+              )
+              expect(table_headers[2]).to eq(
+                ["Supplier Name", "Baked Beans", "1g Big, S",
+                 "Distributor Name",
+                 "3", "10.0", "30.0", "UPS Ground"]
+              )
+              expect(table_headers[3]).to eq(["", "", "", "TOTAL", "3", "", "30.0", ""])
+            end


Is this correct? Issue #9678 is closed.

Yes indeed - thanks @dacook. Inconsistent results ordering is now fixed and the spec is now updated.

Sets up an enterprise user instead of an admin user

Moves file download into report helper Removes pdf file assertation Removes test on PDF file on sales tax report Removes PDF testing from helper

This spec was appearently flaky, let's see if this change stabilizes it. It came up here: https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/10639846576/job/29498582671?pr=12790 Removes CSV tests based on permissions Not sure we need these tests, proposing to remove them and use shared examples to test file download

into its helper, which touches other system specs (namely orders_and_fulfillment_spec.rb). Rubocop fixup

The datet-time-picker test case was failing for me locally, but passing on GH-Actions. Controlling the time should prevent this type of flakyness

The pending was not signalling the bug fix as ordering needed to be corrected

filipefurtad0 · 2024-09-10T00:18:55Z

Failing spec is this one here.

dacook

👍

mkllnk

I have a couple of suggestions but they are only a nice-to-have. I'm much more looking forward to seeing the unit tests. 😉

mkllnk · 2024-09-10T02:35:29Z

spec/support/reports_helper.rb

+  def table_headers
+    rows = find("table.report__table").all("thead tr")
+    rows.map { |r| r.all("th").map { |c| c.text.strip } }
+  end


Be aware that this method name is in the global scope in specs. If another part of the specs wanted to read the table headers of a different table then someone may define a method with the same name and there would be a collision. It would be better to name this report_table_headers.

Also, do you need the find?

Suggested change

def table_headers

rows = find("table.report__table").all("thead tr")

rows.map { |r| r.all("th").map { |c| c.text.strip } }

end

def report_table_headers

rows = all("table.report__table thead tr")

rows.map { |r| r.all("th").map { |c| c.text.strip } }

end

mkllnk · 2024-09-10T03:00:19Z

spec/system/admin/reports/orders_and_distributors_spec.rb

+    around do |example|
+      Timecop.travel(completed_at) { example.run }
+    end


This is not freezing time. It's just going to the beginning of the second, I guess. Therefore flakiness is less likely but not impossible.

github-actions bot assigned filipefurtad0 Aug 19, 2024

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch from 37c3368 to 33642c0 Compare August 19, 2024 22:22

filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Aug 19, 2024

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 3 times, most recently from 0d28666 to 20daddc Compare August 20, 2024 21:19

filipefurtad0 mentioned this pull request Aug 20, 2024

UX improvements for creation of new products [OFN-12744] #12760

Merged

2 tasks

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 3 times, most recently from 31bfc6e to b7ab594 Compare August 27, 2024 00:46

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 3 times, most recently from d75690e to 634a7d6 Compare August 27, 2024 02:24

filipefurtad0 mentioned this pull request Aug 29, 2024

Removes test on PDF file on sales tax report #12830

Merged

4 tasks

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 5 times, most recently from 39911b3 to 67c0ccd Compare September 2, 2024 22:24

mkllnk approved these changes Sep 3, 2024

View reviewed changes

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 2 times, most recently from 80172db to 50da07b Compare September 3, 2024 23:16

dacook requested changes Sep 4, 2024

View reviewed changes

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch 4 times, most recently from 3b26de0 to 76ad152 Compare September 5, 2024 22:55

filipefurtad0 requested a review from dacook September 6, 2024 03:27

filipefurtad0 added no-staging-UK A tag which does not trigger deployments, indicating a server is being used and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used labels Sep 6, 2024

dacook approved these changes Sep 8, 2024

View reviewed changes

filipefurtad0 added 10 commits September 9, 2024 18:07

Moves report test case into dedicated file

fd45dea

Sets up an enterprise user instead of an admin user

Adds coverage for table contents

2a1d494

Adds test case around hub filters

32e3211

Adds basic coverage on report file download

2e5c526

Moves file download into report helper Removes pdf file assertation Removes test on PDF file on sales tax report Removes PDF testing from helper

Adds flatpickr test

ec4dba7

Addresses reviews. The biggest change is moving the table CSS

8500f6c

into its helper, which touches other system specs (namely orders_and_fulfillment_spec.rb). Rubocop fixup

Addresses Davids review

632184b

Adds timer restriction with Timecop

b7aaab2

The datet-time-picker test case was failing for me locally, but passing on GH-Actions. Controlling the time should prevent this type of flakyness

Removes pending from fixed issue

556539d

The pending was not signalling the bug fix as ordering needed to be corrected

filipefurtad0 force-pushed the revisit_Orders_and_Distributors_report branch from 67a3145 to 556539d Compare September 10, 2024 00:09

dacook approved these changes Sep 10, 2024

View reviewed changes

dacook requested a review from mkllnk September 10, 2024 00:21

mkllnk approved these changes Sep 10, 2024

View reviewed changes

rioug merged commit 6849155 into openfoodfoundation:master Sep 11, 2024
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves test coverage on Orders and Distributors report #12790

Improves test coverage on Orders and Distributors report #12790

filipefurtad0 commented Aug 19, 2024 •

edited

Loading

filipefurtad0 commented Aug 27, 2024

filipefurtad0 commented Aug 29, 2024

mkllnk left a comment

mkllnk Sep 3, 2024

mkllnk Sep 3, 2024

filipefurtad0 Sep 3, 2024

mkllnk Sep 3, 2024

filipefurtad0 Sep 3, 2024

mkllnk Sep 3, 2024

filipefurtad0 Sep 3, 2024

filipefurtad0 commented Sep 3, 2024

dacook left a comment

dacook Sep 4, 2024

filipefurtad0 Sep 4, 2024

dacook Sep 4, 2024

dacook left a comment

dacook Sep 8, 2024

filipefurtad0 Sep 9, 2024

filipefurtad0 commented Sep 10, 2024

dacook left a comment

mkllnk left a comment

mkllnk Sep 10, 2024

mkllnk Sep 10, 2024

		visit admin_reports_path
		click_link "Orders And Distributors"

		rows = find("table.report__table").all("thead tr")
		table_headers = rows.map { \|r\| r.all("th").map { \|c\| c.text.strip } }

		it_behaves_like "reports generated as", "CSV", "csv"
		it_behaves_like "reports generated as", "Spreadsheet", "xlsx"

		rows = find("table.report__table").all("tbody tr")
		table = rows.map { \|r\| r.all("td").map { \|c\| c.text.strip } }

Improves test coverage on Orders and Distributors report #12790

Improves test coverage on Orders and Distributors report #12790

Conversation

filipefurtad0 commented Aug 19, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

filipefurtad0 commented Aug 27, 2024

filipefurtad0 commented Aug 29, 2024

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

filipefurtad0 commented Sep 3, 2024

dacook 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

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Sep 10, 2024

dacook left a comment

Choose a reason for hiding this comment

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

filipefurtad0 commented Aug 19, 2024 •

edited

Loading