Move trends plots inside of "Custom" section #3404

julieg18 · 2023-03-06T15:57:07Z

1/3 main <= this <= #3466 <= #3491

Demo

Screen.Recording.2023-03-11.at.6.03.48.PM.mov

Part of #3373

extension/src/plots/model/collect.ts

extension/src/plots/model/custom.ts

webview/src/plots/components/customPlots/CustomPlot.tsx

extension/src/plots/model/collect.ts

extension/src/plots/model/index.ts

extension/resources/walkthrough/plots.md

julieg18 · 2023-03-11T23:51:32Z

extension/src/plots/model/collect.test.ts

@@ -3,25 +3,31 @@ import omit from 'lodash.omit'
 import isEmpty from 'lodash.isempty'


The test coverage on the diff in this pull request is 91.2% (85% is the threshold).

I looked over the code and everything looked pretty covered. Any thing I could have missed to improve the coverage?

julieg18 · 2023-03-11T23:54:01Z

webview/src/plots/components/App.test.tsx

    renderAppWithOptionalData({
-      checkpoint: checkpointPlotsFixture
+      comparison: comparisonTableFixture,
+      custom: customPlotsFixture


custom won't render currently without additional data. Planning to changing that and will adjust tests according.

julieg18 · 2023-03-17T21:48:43Z

I believe #3404 (comment) has been resolved with #3466 and #3491 (both approved and ready to be merged into this pr)!

Here's a (hopefully complete) list of what's left:

Delete old Persistance metrics state (Move trends plots inside of "Custom" section #3404 (comment))
Adjust custom plots order state (Move trends plots inside of "Custom" section #3404 (comment))
Investigate test (Move trends plots inside of "Custom" section #3404 (comment))
Update walkthrough images after other PRs are merged (https://github.com/iterative/vscode-dvc/pull/3404/files#r1133343393)
Check if spec creation can be moved to extension (Move trends plots inside of "Custom" section #3404 (comment))
Consolidate type checking to the smallest space/amount of functions (Move trends plots inside of "Custom" section #3404 (comment)) (Most of this has been done already but more to do)
Add code to add 1 trend and 1 metric vs param custom plot to e2e test (https://github.com/iterative/vscode-dvc/pull/3404/files#r1133364032)

Anything else that should get taken care of before we merge this? Or did I miss anything when I took care of merge conflicts?

sroy3

I think you used to have something that blocked the following from happening before:

Screen.Recording.2023-03-20.at.8.46.26.AM.mov

Could be easy to just show the available choice from the list. (Can be a follow-up)

julieg18 · 2023-03-20T13:47:16Z

I think you used to have something that blocked the following from happening before:
Could be easy to just show the available choice from the list. (Can be a follow-up)

Thanks for spotting that! Simple bug in the quick pick logic/tests that I missed earlier. Will fix it before merging since I'd prefer to stop users' custom plot state having copies of the same plot in it :) Will keep current logic for now and save choosing available choices for a followup.

julieg18 · 2023-03-20T15:03:16Z

Code climate seems to be failing with

ERROR: Coverage for lines (94.9%) does not meet global threshold (95%)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "cover:report" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: The process '/usr/bin/xvfb-run' failed with exit code 1
/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:592
                error = new Error(`The process '${this.toolPath}' failed with exit code ${this.processExitCode}`);
                        ^

Error: The process '/usr/bin/xvfb-run' failed with exit code 1
    at ExecState._setResult (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:592:25)
    at ExecState.CheckComplete (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:575:18)
    at ChildProcess.<anonymous> (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:469:27)
    at ChildProcess.emit (node:events:527:28)
    at maybeClose (node:internal/child_process:1092:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
Error: 🚨 Coverage run failed!

First guess is the issue is amount of lines being covered:

ERROR: Coverage for lines (94.9%) does not meet global threshold (95%)

Will see if upping the code coverage fixes the issue!

julieg18 · 2023-03-20T17:26:40Z

Updated the tests based of code reviews and looking over things myself:

fixed pickMetric not being 100% covered
added tests for addCustomPlot and removeCustomPlot ending early due to user not selecting things
added additional tests for functions in custom.test.ts that appeared to be missing coverage

But code coverage is still failing just shy of 95%:

ERROR: Coverage for lines (94.96%) does not meet global threshold (95%)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: "cover:report" exited with 1.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:592
Error: The process '/usr/bin/xvfb-run' failed with exit code 1
                error = new Error(`The process '${this.toolPath}' failed with exit code ${this.processExitCode}`);
                        ^
Error: The process '/usr/bin/xvfb-run' failed with exit code 1
    at ExecState._setResult (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:592:25)
    at ExecState.CheckComplete (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:575:18)
    at ChildProcess.<anonymous> (/home/runner/work/_actions/paambaati/codeclimate-action/v3.2.0/node_modules/@actions/exec/lib/toolrunner.js:469:27)
    at ChildProcess.emit (node:events:527:28)
    at maybeClose (node:internal/child_process:1092:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
Error: 🚨 Coverage run failed!

Last things I'm seeing regarding testing is:

Add tests for lines like this in collect.test.ts: exp.name || exp.label
Add tests for when user recreates the same custom plot

I'll start working on the followup on stopping a user from recreating custom plots since I think it will take about the same time as just creating tests for our current solution. But is there something else that could be going on with CodeClimate that I'm missing? cc @sroy3, @mattseddon

sroy3 · 2023-03-20T17:40:41Z

I'll start working on the followup on stopping a user from recreating custom plots since I think it will take about the same time as just creating tests for our current solution. But is there something else that could be going on with CodeClimate that I'm missing? cc @sroy3, @mattseddon

We've merged too many things with low code coverage. We've pretty much hit the limit where we either get 100% coverage on a PR or else coverage will fail. I'll try to add coverage where it's missing if possible. Other than that, we have 2 other options from now on. Always hitting 100% coverage until the situation gets better or lower threshold to something lower (90%?).

extension/src/plots/model/index.ts

codeclimate · 2023-03-20T23:01:10Z

Code Climate has analyzed commit 2945e34 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2
Duplication	3

The test coverage on the diff in this pull request is 96.4% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.0%.

View more on Code Climate.

Move trends inside of custom

60ae393

julieg18 added the product PR that affects product label Mar 6, 2023

julieg18 self-assigned this Mar 6, 2023

julieg18 added 3 commits March 6, 2023 10:03

fix typo

6dc8813

fix linting

88fe26d

Fix jest tests

c325008

mattseddon mentioned this pull request Mar 8, 2023

Select the number of items per row from slider #3405

Merged

julieg18 added 2 commits March 8, 2023 10:17

Fix vscode tests

6216af0

Fix reordering bug

e505442

codeclimate bot reviewed Mar 8, 2023

View reviewed changes

extension/src/plots/model/collect.ts Outdated Show resolved Hide resolved

extension/src/plots/model/custom.ts Outdated Show resolved Hide resolved

julieg18 added 2 commits March 8, 2023 18:41

Resolve comments

8ab22c4

Merge branch 'main' into move-trends-inside-custom

64befd1

codeclimate bot reviewed Mar 9, 2023

View reviewed changes

webview/src/plots/components/customPlots/CustomPlot.tsx Outdated Show resolved Hide resolved

julieg18 and others added 11 commits March 9, 2023 09:13

Fix e2e tests

3354b3e

Add accidentally deleted test

bd433cb

Rename trends plot

b4d64b3

Refactor

7052633

Delete unused metrics code

3bd3bc1

Add extra quick pick tests

1a6e423

Update readme

359a20c

Add test for creating both kinds of custom plots

c9ea4fe

Fix possibly broken old state

e08e30e

Merge branch 'main' into move-trends-inside-custom

cd626b9

Improve text

e3eba7b

julieg18 commented Mar 11, 2023

View reviewed changes

Refactor

74bc253

julieg18 marked this pull request as ready for review March 12, 2023 00:33

julieg18 requested a review from mattseddon as a code owner March 12, 2023 00:33

julieg18 added 2 commits March 17, 2023 12:31

Merge branch 'main' into move-trends-inside-custom

71ed367

Replace checkpoint with custom in App.test.tsx

7ebebdf

julieg18 marked this pull request as ready for review March 17, 2023 21:49

Merge branch 'main' into move-trends-inside-custom

9630fdd

julieg18 requested review from sroy3 and mattseddon March 17, 2023 21:49

sroy3 approved these changes Mar 20, 2023

View reviewed changes

julieg18 and others added 4 commits March 20, 2023 09:01

Fix "plot already exists" logic

40d0010

Merge branch 'main' into move-trends-inside-custom

9ae29fd

Consolidate collectCustomPlots (#3466)

1ab409c

Merge branch 'main' into move-trends-inside-custom

3c92cc4

julieg18 and others added 4 commits March 20, 2023 10:32

Add tests for custom.ts

a2b9c75

Fix typo in tests

418ee53

Add tests for user ending early on +/- custom plots

bd305a1

Merge branch 'main' into move-trends-inside-custom

897d60c

Add testing for name || label code

4032f1c

Merge branch 'main' into move-trends-inside-custom

ea8e4b3

codeclimate bot reviewed Mar 20, 2023

View reviewed changes

extension/src/plots/model/index.ts Show resolved Hide resolved

Merge branch 'main' into move-trends-inside-custom

c3e4d6f

julieg18 enabled auto-merge (squash) March 20, 2023 22:54

Merge branch 'main' into move-trends-inside-custom

2945e34

julieg18 merged commit 6cfade1 into main Mar 20, 2023

julieg18 deleted the move-trends-inside-custom branch March 20, 2023 23:09

julieg18 mentioned this pull request Mar 20, 2023

Improve Custom Plots #3373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move trends plots inside of "Custom" section #3404

Move trends plots inside of "Custom" section #3404

julieg18 commented Mar 6, 2023 •

edited

Loading

julieg18 Mar 11, 2023

julieg18 Mar 11, 2023

julieg18 commented Mar 17, 2023 •

edited

Loading

sroy3 left a comment

julieg18 commented Mar 20, 2023 •

edited

Loading

julieg18 commented Mar 20, 2023

julieg18 commented Mar 20, 2023 •

edited

Loading

sroy3 commented Mar 20, 2023

codeclimate bot commented Mar 20, 2023

		@@ -3,25 +3,31 @@ import omit from 'lodash.omit'
		import isEmpty from 'lodash.isempty'

Move trends plots inside of "Custom" section #3404

Move trends plots inside of "Custom" section #3404

Conversation

julieg18 commented Mar 6, 2023 • edited Loading

1/3 main <= this <= #3466 <= #3491

Demo

julieg18 Mar 11, 2023

Choose a reason for hiding this comment

julieg18 Mar 11, 2023

Choose a reason for hiding this comment

julieg18 commented Mar 17, 2023 • edited Loading

sroy3 left a comment

Choose a reason for hiding this comment

julieg18 commented Mar 20, 2023 • edited Loading

julieg18 commented Mar 20, 2023

julieg18 commented Mar 20, 2023 • edited Loading

sroy3 commented Mar 20, 2023

codeclimate bot commented Mar 20, 2023

julieg18 commented Mar 6, 2023 •

edited

Loading

julieg18 commented Mar 17, 2023 •

edited

Loading

julieg18 commented Mar 20, 2023 •

edited

Loading

julieg18 commented Mar 20, 2023 •

edited

Loading