fixes #5954 feat(nimbus): group home page experiment tabs by all statuses #5969

jodyheavener · 2021-07-15T19:53:11Z

This PR changes the directory/home page view to display for all experiment lifecycle statuses, instead of grouping them together.

As a bonus, I also added a mechanism to remember which tab you were last on.

jaredlockhart

Awesome! Thank you @jodyheavener , so much easier to navigate. The session thing is a neat idea but please pull that out of this pr and file a separate ticket for it, we explicitly wanted it to always land on Live on a fresh load. We should look at the idea of storing the current tab separately. I'd like it to persist in the URL, we can look at that separately.

pdehaan · 2021-07-15T20:22:29Z

app/experimenter/nimbus-ui/src/components/PageHome/DirectoryTable/index.test.tsx

@@ -105,14 +105,10 @@ function expectTableCells(testId: string, cellTexts: string[]) {
 }

 describe("DirectoryTable", () => {
-  it("renders as expected with default columns", () => {
+  fit("renders as expected with default columns", async () => {


Re: it => fit,

What dat about? I'm a bit unclear after https://jestjs.io/docs/api#testonlyname-fn-timeout about which tests will be run with that tweaked. If this describe("DirectoryTable", ...) suite has three it tests set, but one of them is fit, does that mean it will only run the 1 of the 3 tests now?

✅ fit("renders as expected with default columns", async () => {...}
❌ it("renders as expected without experiments", () => {...}
❌ it("renders as expected with custom columns", () => {...}

Whoops! Leftover from testing. Good catch.

jaredlockhart

Yeyeyeyeyeye hypehypehypehype 🙏 🙏 🙏 🙏 🙏

LZoog

Also LGTM! Thanks for the Storybook refactoring to the new pattern. 😁

pdehaan

Preemptively requesting changes until the integration tests are greenish.

pdehaan · 2021-07-16T19:55:26Z

app/tests/integration/nimbus/pages/home.py

@@ -7,7 +7,7 @@ class HomePage(Page):
    """Nimbus Home page."""

    _create_new_btn_locator = (By.CSS_SELECTOR, "#create-new-button")
-    _page_wait_locator = (By.CSS_SELECTOR, ".table")
+    _page_wait_locator = (By.CSS_SELECTOR, ".directory-table")


I might not be understanding the exact role of data-testid.
Above we have the following snippet:

<table className="table" data-testid="DirectoryTable">

I don't know I'd expect this CSS selector to work if we're only setting a .table class, but then trying to use .directory-table here. Not sure if the selector should be something weird like table.table[data-testid="DirectoryTable"] or if just [data-testid="DirectoryTable"] works here (or heck, even just .table since we're now following The Highlander rules of "there can be only one").

But now that we've split the 3 sub-tables on live into separate tabs, the tests might need to be tweaked further if we were trying to do a lookup of experiments in "Review" or "Preview" states. /cc @jrbenny35

jaredlockhart · 2021-07-16T21:25:37Z

@jodyheavener rebasing this on main may clear up the integration failure

jodyheavener · 2021-07-19T15:13:35Z

@jrbenny35 @pdehaan I'm a little stuck on these integration test failures. It appears to be having trouble finding the element to indicate the page is loaded. This was previously .table, but since that element is now conditionally rendered I switched it to .directory-table, an element that is always visible when the experiment request has finished. It is unable to find this element. I even tried using #PageHome-page, an element that is immediately visible, as the selector just to see if it could find that, with no luck. Kinda stumped 🤔

pdehaan · 2021-07-19T16:23:57Z

Ah, curious, so we should always have a .directory-table wrapper div, but don't always have a .table since we now conditionally render it based on experiments.length, and otherwise render <p data-testid="no-experiments">No experiments found.</p>.

It looks like the current integration_nimbus error is:

nimbus/test_e2e_create_experiment.py:118: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
nimbus/pages/home.py:23: in tables
    self.wait.until(EC.presence_of_element_located(_table_locator))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

experimenter/app/tests/integration/nimbus/pages/home.py

Lines 20 to 25 in 568f4cf

    
           @property 
        
           def tables(self): 
        
               _table_locator = (By.CSS_SELECTOR, ".active .directory-table .table") 
        
               self.wait.until(EC.presence_of_element_located(_table_locator)) 
        
               tables = self.find_elements(*_table_locator) 
        
               return [self.Tables(self, el) for el in tables]

So, re: _table_locator = (By.CSS_SELECTOR, ".active .directory-table .table"), I see the .directory-table and [conditional] .table class names below:

    <div className="directory-table pb-2 mt-4">
      {experiments.length ? (
        <table className="table" data-testid="DirectoryTable">

I'll have to poke around a bit more and see where the .active className is being added. I'm guessing that's the active section tab, which I don't expect changed behavior (or I didn't see it in this PR).

jodyheavener · 2021-07-19T16:50:55Z

@pdehaan Oh that's interesting, locally my tests fail with:

nimbus/test_e2e_create_experiment.py:236:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
nimbus/pages/home.py:16: in wait_for_page_to_load
    self.wait.until(EC.visibility_of_element_located(self._page_wait_locator))

pdehaan · 2021-07-19T17:06:58Z

nimbus/pages/home.py:16: in wait_for_page_to_load
   self.wait.until(EC.visibility_of_element_located(self._page_wait_locator))

Curious, since that code is now:

_page_wait_locator = (By.CSS_SELECTOR, ".directory-table")

And I thought that <div className="directory-table pb-2 mt-4"> always existed. I might have to defer to @jrbenny35's expertise here in adding some pauses or screenshots to help debug locally.

jaredlockhart · 2021-07-20T16:26:18Z

Eyyyyyy green finally 🍏 📗 🟢 💚 🥗

jrbenny35 · 2021-07-20T18:10:39Z

app/tests/integration/nimbus/test_e2e_create_experiment.py

@@ -124,7 +124,7 @@ def test_create_new_experiment_remote_settings(selenium, base_url):
    # Check it's live
    home = HomePage(selenium, base_url).wait_for_page_to_load()
    live_experiments = home.tables[0]
-    assert "live experiments" in live_experiments.table_name.lower()
+    assert home.active_tab_text, "Live"


This should check that the active tab has "live experiments" within it, currently, this assert will always pass.

See: https://www.w3schools.com/python/ref_keyword_assert.asp

@jrbenny35 Hmm, would it always pass? I want to assert that the tab is selected (contains the .active class). So if "Review" was selected the active_tab_text would not equal "Live". I changed it to this since we removed the element/text that previously displayed "live experiments"

Edit: oh I see what you meant about the usage of assert. Updated that, but I'm still checking for "Live" in the tab text and since "Live experiments" no longer exists.

pdehaan

Great job!
Just 2 small questions.

pdehaan · 2021-07-20T23:19:40Z

app/experimenter/nimbus-ui/src/components/PageHome/DirectoryTable/index.test.tsx

@@ -105,14 +105,10 @@ function expectTableCells(testId: string, cellTexts: string[]) {
 }

 describe("DirectoryTable", () => {
-  it("renders as expected with default columns", () => {
+  it("renders as expected with default columns", async () => {


Q: Why did we add async here?
I think this is the only test in this file that is async (but I don't see any await or returning a promise.

Whoops, must have thought I was going to add an async method here and then didn't. Will update.

pdehaan · 2021-07-20T23:30:51Z

app/experimenter/nimbus-ui/src/components/PageHome/DirectoryTable/index.tsx

-                  className={`border-top-0 ${
-                    i === 0 ? "font-weight-bold" : "font-weight-normal"
-                  }`}
+                  className="border-top-0"
                  key={label}
                  data-testid="directory-table-header"


Q: does a data-testid have to be unique?
Not a regression or change, more of a personal curiosity and didn't see any obvious answers on testing-library docs.

Q: does a data-testid have to be unique?

Nope! You can use the queryAllBy* / findAllBy* / getAllBy* methods to look up multiple elements of the same criteria. In fact, we are doing exactly that in this test file.

…uses

pdehaan

jodyheavener requested review from lmorchard, jaredlockhart and LZoog July 15, 2021 19:53

jaredlockhart requested changes Jul 15, 2021

View reviewed changes

pdehaan reviewed Jul 15, 2021

View reviewed changes

jodyheavener force-pushed the jh/5954/directory-tabs branch 2 times, most recently from bfcecc5 to 394a089 Compare July 16, 2021 13:03

jodyheavener requested a review from jaredlockhart July 16, 2021 13:06

jaredlockhart approved these changes Jul 16, 2021

View reviewed changes

jodyheavener force-pushed the jh/5954/directory-tabs branch from 394a089 to 92f3940 Compare July 16, 2021 14:53

jodyheavener requested a review from jrbenny35 as a code owner July 16, 2021 14:53

jodyheavener force-pushed the jh/5954/directory-tabs branch from 92f3940 to 698fd23 Compare July 16, 2021 15:38

LZoog approved these changes Jul 16, 2021

View reviewed changes

pdehaan requested changes Jul 16, 2021

View reviewed changes

jodyheavener force-pushed the jh/5954/directory-tabs branch from 698fd23 to 4aabcb9 Compare July 19, 2021 13:11

jodyheavener force-pushed the jh/5954/directory-tabs branch from 4aabcb9 to 9b893f1 Compare July 19, 2021 15:35

jodyheavener force-pushed the jh/5954/directory-tabs branch 6 times, most recently from 976b7f8 to 51b5ba1 Compare July 20, 2021 15:56

jodyheavener requested a review from pdehaan July 20, 2021 16:06

jrbenny35 requested changes Jul 20, 2021

View reviewed changes

jodyheavener requested a review from jrbenny35 July 20, 2021 18:30

pdehaan reviewed Jul 21, 2021

View reviewed changes

fixes #5954 feat(nimbus): group home page experiment tabs by all stat…

f476c08

…uses

jodyheavener force-pushed the jh/5954/directory-tabs branch from a0464c9 to f476c08 Compare July 21, 2021 13:15

jrbenny35 approved these changes Jul 21, 2021

View reviewed changes

jodyheavener requested a review from pdehaan July 21, 2021 13:28

pdehaan approved these changes Jul 21, 2021

View reviewed changes

jodyheavener merged commit 3d3f3bc into main Jul 21, 2021

jodyheavener deleted the jh/5954/directory-tabs branch July 21, 2021 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #5954 feat(nimbus): group home page experiment tabs by all statuses #5969

fixes #5954 feat(nimbus): group home page experiment tabs by all statuses #5969

jodyheavener commented Jul 15, 2021

jaredlockhart left a comment

pdehaan Jul 15, 2021

jodyheavener Jul 16, 2021

jaredlockhart left a comment

LZoog left a comment

pdehaan left a comment

pdehaan Jul 16, 2021

jaredlockhart commented Jul 16, 2021

jodyheavener commented Jul 19, 2021

pdehaan commented Jul 19, 2021

jodyheavener commented Jul 19, 2021

pdehaan commented Jul 19, 2021

jaredlockhart commented Jul 20, 2021

jrbenny35 Jul 20, 2021

jodyheavener Jul 20, 2021 •

edited

pdehaan left a comment

pdehaan Jul 20, 2021

jodyheavener Jul 21, 2021

pdehaan Jul 20, 2021

jodyheavener Jul 21, 2021

pdehaan left a comment

fixes #5954 feat(nimbus): group home page experiment tabs by all statuses #5969

fixes #5954 feat(nimbus): group home page experiment tabs by all statuses #5969

Conversation

jodyheavener commented Jul 15, 2021

jaredlockhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredlockhart left a comment

Choose a reason for hiding this comment

LZoog left a comment

Choose a reason for hiding this comment

pdehaan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredlockhart commented Jul 16, 2021

jodyheavener commented Jul 19, 2021

pdehaan commented Jul 19, 2021

jodyheavener commented Jul 19, 2021

pdehaan commented Jul 19, 2021

jaredlockhart commented Jul 20, 2021

Choose a reason for hiding this comment

jodyheavener Jul 20, 2021 • edited

Choose a reason for hiding this comment

pdehaan 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

pdehaan left a comment

Choose a reason for hiding this comment

jodyheavener Jul 20, 2021 •

edited