Unit test for browsing page by shubham-html-css-js · Pull Request #2851 · distributed-system-analysis/pbench

shubham-html-css-js · 2022-05-16T16:27:35Z

Configured Package.json to support unit testing without interfering with Patternfly icon import and wrote unit tests for some browsing page components

shubham-html-css-js · 2022-05-16T16:36:00Z

I don't know why it still says I want to push 30 commits even though all other commits have been merged(I am still learning github),but we have previous commits merged,so you just have to review the latest commit related to unit test

portante

30 commits for an initial PR might indicate that the starting point was not from the already merged code base in branch main.

Merge conflicts are being reported for this PR, so perhaps we could have it rebased?

dbutenhof

Great start, Shubham! Thanks for working on the unit tests; it's not always as much fun as new features, but it's important!

It sounds like you pushed this PR from a local branch where you'd already been working, without rebasing against main. The rebase should make them go away, so long as they really were merged.

I usually create a new branch from upstream main when starting a PR instead of re-using one; if you reuse, be sure you update your main and rebase first.

dbutenhof · 2022-05-16T19:42:31Z

dashboard/src/modules/components/DatePickerComponent/DatePicker.test.js

@@ -0,0 +1,30 @@
+import { Provider } from "react-redux";
+import store from "store/store";
+import { MOCK_DATA } from "utils/mockData";


This is specifically a mock for /api/v1/datasets/list with specific metadata requested. I think it would be better to give this a more specific name than just "MOCK_DATA", especially with the suggestion that a file named "mockData.js" might provide mocks for other APIs as well?

dashboard/src/modules/components/DatePickerComponent/DatePicker.test.js

dashboard/src/modules/components/SearchComponent/Search.test.js

dashboard/src/modules/components/TableComponent/Table.test.js

webbnh · 2022-05-18T19:31:26Z

I don't know why it still says I want to push 30 commits even though all other commits have been merged(I am still learning github),but we have previous commits merged,so you just have to review the latest commit related to unit test

Based on the GitHub "Network" graph on the "Insights" page, it would appear that you created your unit_test_browsing_page branch from the end of your browsing_page_dashboard_dev branch, and that has about 30 commits on it since it branched from main. So, your PR, which requests merging the unit_test_browsing_page branch into main, has 30 commits to merge. Now, hopefully, the contents of all but the last of them would have already been present, so the merge would be a no-op, but that's not evident until the merge is attempted (and, even if it would be a no-op in the end, the individual merges might have been painful). Now, you have since merged main into your unit_test_browsing_page branch, which should make the subsequent merge into main fairly painless, but this isn't the usual route that we take.

When you start new work, you should start a new branch, and generally that branch should start from the current end of the main branch. (So, before checking out and creating your new branch, you should issue a command like git remote update (and, I like to add the --prune switch to remove local tracking branches for upstream branches which have been deleted). And, then, you should make sure that it's the main branch that you're checking out from. (You can reuse local development branches, but there's generally no need to, and it doesn't generally fit well with our preference for a "linear history".)

Second, you generally should not merge from main to your development branch. If there is something on main that you need, or if your development branch is otherwise out of date, you should rebase your development branch, in effect "moving" the root of it to the new end of the main branch, as thought it had been checked out from there initially. (This process has effects similar to merging from the main branch (including the possibility of having to manually resolve merge conflicts), but it produces a different, linear history, and it leaves your branch in a state where it can be rebased again, as needed.)

As for your present situation, there are several ways that you could recover. One would be to reset your branch back prior to the merge from main, and then rebase it, changing the root from browsing_page_dashboard_dev branch to the end of main (this is a fairly advanced but fairly straightforward operation). But, since you've only got one commit that you're interested in, the simplest thing would be to create a new branch rooted at the end of main and "cherry-pick" the one commit from your old branch on to it.

If you rename your old branch first, and then create the new branch with the old name, I believe that you'll be able to force-push it to your fork and it will replace the branch there...and the PR will be updated with the result.

webbnh

Shubham, this is a great start!

However, I'm thinking that this PR should be split: there are a number of pre-cursor changes which are not directly related to unit testing, although they provide features which the unit testing depends on; they should be pulled out into a separate PR which this PR should be based on.

You do a lot of searching for UI elements by various bits of text. These searches seem to be done via case-blind regular expressions containing literal strings. It seems like we should know the exact text, so regular expressions (particularly the case-mapping) shouldn't be necessary. And, it also seems like there should be a more rigorous way of addressing these elements than by picking bits of UI text...but, since that's what the user actually sees, perhaps that's just the thing, but...it seems odd to me.

And, I've got a few other nits, comments, suggestions, and questions.

dashboard/package.json

webbnh · 2022-05-18T19:56:43Z

dashboard/src/App.js

  useEffect(() => {
    const faviconLogo = document.getElementById("favicon");
-    faviconLogo.setAttribute("href", favicon);
+    faviconLogo?.setAttribute("href", favicon);


This change doesn't seem like it should be in a unit testing commit/PR.

webbnh · 2022-05-18T19:58:11Z

dashboard/src/modules/components/AlertComponent/Alert.test.js

@@ -0,0 +1,26 @@
+import { Provider } from "react-redux";
+import store from "store/store";
+import App from "../../../App";


In this context, do we have a "root" configured, so that we don't have to use relative import paths? (I.e., can we just import this from "App" or from ./Appbecause the root issrc`?) Ditto for the other modules.

dashboard/src/modules/components/AlertComponent/Alert.test.js

webbnh · 2022-05-18T20:29:17Z

dashboard/src/modules/components/AlertComponent/Alert.test.js

+test("Alert message is displayed on initial load", () => {
+  render(<AppWrapper />);
+  const alert = screen.getByText(/want to see your own data/i);
+  expect(alert).toBeInTheDocument();


Are there any other attributes that we should be checking?

webbnh · 2022-05-18T20:59:51Z

dashboard/src/modules/components/SearchComponent/index.jsx

        onChange={(e) => setControllerValue(e)}
      ></TextInput>
-      <Button variant="control" onClick={searchController}>
+      <Button variant="control" onClick={searchController} aria-label="searchButton">


This doesn't seem like a unit-test-related change.

dashboard/src/modules/components/SearchComponent/Search.test.js

webbnh · 2022-05-18T21:13:10Z

dashboard/src/modules/components/DatePickerComponent/DatePicker.test.js

+test("data is filtered based on date range selected from date picker", async () => {
+  render(<AppWrapper />);
+  await screen.findByText("dhcp1");
+  const datePickerInput = screen.getAllByPlaceholderText(/yyyy-mm-dd/i);


Can you target them by aria-label value? That seems like a more robust discriminator.

webbnh · 2022-05-18T21:14:27Z

dashboard/src/modules/components/DatePickerComponent/DatePicker.test.js

+  render(<AppWrapper />);
+  await screen.findByText("dhcp1");
+  const datePickerInput = screen.getAllByPlaceholderText(/yyyy-mm-dd/i);
+  fireEvent.change(datePickerInput[0], { target: { value: "2022-02-16" } });


Shouldn't we be checking that the UI has updated from YYYY-MM-DD to 2022-02-16?

Also, can the code here look at the values in the React-store? I think those would be good things to check, as well.

dashboard/src/modules/components/HeadingComponent/Heading.test.js

dbutenhof

Thanks, Shubham! I can resolve some of my previous conversations, but Webb has some good points/questions and I'd prefer to see a better name for the mocked data.

dbutenhof · 2022-05-25T20:52:42Z

dashboard/src/utils/mockData.js

@@ -0,0 +1,32 @@
+export const MOCK_DATA = [


I'm still a bit concerned about the generic name MOCK_DATA ... this mocks the response of /datasets/list, but we'll certainly want to have other mocks in the future to test additional parts of the dashboard.

The /datasets/list response format has changed. There's now a "results" field in the JSON as well as a "next_url" for paging. Also, the dashboard now relies on the resource_id.

{ "next_url": "", "results": [ { "metadata": { "dataset.created": "2022-07-27T15:39:06+00:00" }, "name": "pbench-user-benchmark_xyz.22_2022.07.27T15.39.06", "resource_id": "c54ab5929dc6230937402b682751c1ac" }, { "metadata": { "dataset.created": "2022-07-27T15:01:57+00:00" }, "name": "Short timer", "resource_id": "8c19617fc80580eaf6bb6808a1f8c46f" }, { "metadata": { "dataset.created": "2022-07-27T13:45:02+00:00" }, "name": "Soon FOUR", "resource_id": "4e28f76ccc9572f7e6a27898678db39d" }, { "metadata": { "dataset.created": "2022-07-27T13:45:11+00:00" }, "name": "Soonish", "resource_id": "892e506d3302ebaa466b9bc4c02f1780" }, { "metadata": { "dataset.created": "2022-07-20T16:28:32+00:00" }, "name": "Soon ONE", "resource_id": "6a4b55f64f8dd5f997be0da70a5e0d2c" }, { "metadata": { "dataset.created": "2022-07-27T13:44:53+00:00" }, "name": "Soon THREE", "resource_id": "e368132b16f8a45407c77c0a54a427fb" } ], "total": 6 }

dbutenhof · 2022-05-25T20:57:33Z

You need to git fetch upstream main and git rebase (or equivalent) to merge your package.json changes with the latest on main, then git push --force origin unit_test_browsing_page (that's a long branch name!)

webbnh · 2022-05-25T21:49:31Z

You need to git fetch upstream main

I've come to prefer git remote update --prune, but I guess git fetch works too, if you only want to update the single branch. (One of the things that --prune does is remind me to delete my development branches after they've been merged into main.)

that's a long branch name!

I don't know about you, but I've got Git's command line "completer" configured, so I just type a couple of characters and then tab-complete the branch name...so it doesn't matter how long it is. 😉

webbnh

It looks like you got the extraneous commits off your branch (yay!), but apparently your branch has conflicts in dashboard/package.json, so it cannot be merged, and the CI tests are refusing to run; so you should address that as soon as possible. (E.g., update your local tracking branch for main, rebase your branch on the end of main again, and resolve the conflicts during the rebase.)

Otherwise, it's just small stuff, but there are some previous comments of mine which you haven't addressed, yet.

webbnh · 2022-05-25T20:35:31Z

dashboard/src/modules/components/DatePickerComponent/DatePicker.test.js

+  const datasetNameOne = screen.queryByText("pbench_user_benchmark1");
+  const datasetNameTwo = screen.queryByText("pbench_user_benchmark2");
+  const datasetNameThree = screen.queryByText("pbench_user_benchmark3");
+  const datasetNameFour = screen.queryByText("pbench_user_benchmark4");
+  const datasetNameFive = screen.queryByText("pbench_user_benchmark5");
+  expect(datasetNameOne).toBeInTheDocument();
+  expect(datasetNameTwo).toBeInTheDocument();
+  expect(datasetNameThree).toBeInTheDocument();
+  expect(datasetNameFour).not.toBeInTheDocument();
+  expect(datasetNameFive).not.toBeInTheDocument();


It looks to me like these assertions will fit on one line, so there's no need to use local variables for them (in-lining the expressions will cut the number of new lines of code in half...).

expect(screen.queryByText("pbench_user_benchmark1")).toBeInTheDocument(); ... expect(screen.queryByText("pbench_user_benchmark5")).not.toBeInTheDocument();

A similar comment applies to dashboard/src/modules/components/TableComponent/Table.test.js, and perhaps elsewhere.

dashboard/src/modules/components/SearchComponent/index.jsx

webbnh · 2022-05-25T20:47:50Z

dashboard/src/modules/components/TableComponent/Table.test.js

-  waitFor,
-  fireEvent,
-} = require("@testing-library/react");
+const { render, screen, fireEvent } = require("@testing-library/react");


Teach me something: why do we use require() here instead of import?

dashboard/src/modules/components/TableComponent/Table.test.js

webbnh · 2022-05-25T22:10:26Z

dashboard/src/modules/components/TableComponent/index.jsx

  }, [dispatch]);

-  const { publicData, favoriteRepoNames } = useSelector(
+  const { publicData, favoriteRepoNames,tableData } = useSelector(


Looks like you need to run Prettier again. 🙃

It would be good to develop a habit of running Prettier before each commit.

dashboard/package.json

webbnh · 2022-05-25T22:16:20Z

dashboard/public/index.html

      It will be replaced with the URL of the `public` folder during the build.
      Only files inside the `public` folder can be referenced from the HTML.
-
+      


You've got some superfluous blank characters, here.

portante · 2022-05-26T00:14:49Z

Please consider https://opensource.com/article/20/11/multiple-git-repositories. This gives us all a common way to organize and work together using the same names.

You need to git fetch upstream main

While it is nice to target one branch, it is more often helpful to keep track of everything in upstream (when defined as recommended in the blog above), since that avoid missing the creation of remote branches for shared projects or releases.

and git rebase (or equivalent) to merge your package.json changes with the latest on main, then git push --force origin unit_test_browsing_page (that's a long branch name!)

You can also use git push origin +unit_test_browsing_page where the leading + means --force.

dbutenhof · 2022-05-26T19:30:38Z

While it is nice to target one branch, it is more often helpful to keep track of everything in upstream (when defined as recommended in the blog above), since that avoid missing the creation of remote branches for shared projects or releases.

Yup; but I don't use most of those branches routinely, and keeping main up to date is important. I tend to update remotes with --prune once in a while (maybe slightly more often than git gc), while I use git fetch upstream main regularly.

You can also use git push origin +unit_test_browsing_page where the leading + means --force.

Now that I did not know, and that's useful...

portante

Please update commit log message with Jira issue ID "PBENCH-708".

shubham-html-css-js requested review from dbutenhof, npalaska, riya-17 and webbnh May 16, 2022 16:32

portante requested changes May 16, 2022

View reviewed changes

dbutenhof requested changes May 16, 2022

View reviewed changes

webbnh requested changes May 18, 2022

View reviewed changes

shubham-html-css-js force-pushed the unit_test_browsing_page branch 2 times, most recently from fa14411 to 0f1d0ee Compare May 24, 2022 13:04

shubhamBansal added 2 commits May 24, 2022 18:52

Unit tests for browsing page component

ae145bc

Controller notion removed from test cases and support for coverage added

24dbd85

shubham-html-css-js force-pushed the unit_test_browsing_page branch from 0f1d0ee to 24dbd85 Compare May 24, 2022 13:24

Removed case binding match

17d87f2

shubham-html-css-js requested review from dbutenhof and webbnh May 24, 2022 21:05

dbutenhof reviewed May 25, 2022

View reviewed changes

webbnh reviewed May 25, 2022

View reviewed changes

portante requested changes May 28, 2022

View reviewed changes

shubham-html-css-js self-assigned this Jun 5, 2022

shubham-html-css-js added Dashboard Of and relating to the Dashboard GUI Unit tests testing labels Jun 5, 2022

portante added this to the v0.72 milestone Jun 14, 2022

dbutenhof added the test label Dec 12, 2025

dbutenhof added Abandoned test and removed test Abandoned labels Dec 12, 2025

		It will be replaced with the URL of the `public` folder during the build.
		Only files inside the `public` folder can be referenced from the HTML.

Comments

Conversation

shubham-html-css-js commented May 16, 2022

Uh oh!

shubham-html-css-js commented May 16, 2022

Uh oh!

portante left a comment

Choose a reason for hiding this comment

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webbnh commented May 18, 2022

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbutenhof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbutenhof commented May 25, 2022

Uh oh!

webbnh commented May 25, 2022

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

portante commented May 26, 2022

Uh oh!

dbutenhof commented May 26, 2022

Uh oh!

portante left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!