fix(KONFLUX-5886): allow unselecting default context #58

CryptoRodeo · 2024-12-18T19:34:30Z

The user should be able to unselect the default context during the creation of an integration test.

Other changes:

Contexts are now required (at least one)

CryptoRodeo · 2024-12-18T19:39:40Z

Demo of the following:

default context can now be unselected (main bug fixed)
contexts are now required by the form and modal (at least 1 must be selected).

konflux-5886.mp4

CryptoRodeo · 2024-12-18T21:50:49Z

It appears that the code coverage step fails:

error - 2024-12-18 21:48:17,362 -- Report creating failed: {"message":"Token required because branch is protected"}
Error: Codecov:
                        Failed to properly create report: The process '/home/runner/work/_actions/codecov/codecov-action/v4/dist/codecov' failed with exit code 1

testcara · 2024-12-19T00:43:51Z

I met the same token problem for my ci jobs but after 2 days later, retry fixed the problem. It seems the ci is not very stable for this part.

testcara · 2024-12-19T00:44:09Z

LGTM

JoaoPedroPP · 2025-01-06T20:20:29Z

LGTM

StanislavJochman · 2025-01-07T23:04:37Z

LGTM

sahil143 · 2025-01-08T06:19:26Z

src/components/IntegrationTests/utils/test-utils.ts

@@ -0,0 +1,17 @@
+import { fireEvent, screen, act } from '@testing-library/react';


move this file under __tests__?

File has been moved

Summary of all failing tests FAIL src/components/IntegrationTests/__tests__/test-utils.ts ● Test suite failed to run Your test suite must contain at least one test. at onResult (node_modules/@jest/core/build/TestScheduler.js:133:18) at node_modules/@jest/core/build/TestScheduler.js:254:19 at node_modules/emittery/index.js:363:13 at Array.map (<anonymous>) at Emittery.emit (node_modules/emittery/index.js:361:23)

It looks like the test run failed because only tests are expected under __tests__... would moving this back to utils be fine? Or maybe __test-utils__?

@sahil143 I reverted the change to fix the unit test step from failing (see above). If you have any recommendations on where else I should move this besides __tests__ to prevent the failure again please let me know.

My only concern is that the testing utilities should not be kept with the actual component utilities. I like the idea of creating separate utilities for opening dropdowns that can be used everywhere else. You can rename the functions to something more specific like openIntegrationTestContextDropown and move them to the common test-utils in src/utils/test-utils.ts. I am open to any other suggestions.

codecov · 2025-01-09T21:20:47Z

Codecov Report

Attention: Patch coverage is 83.63636% with 9 lines in your changes missing coverage. Please review.

Project coverage is 80.09%. Comparing base (88eafd6) to head (b06bc0f).

Files with missing lines	Patch %	Lines
...ests/IntegrationTestForm/utils/validation-utils.ts	42.85%	4 Missing ⚠️
...onents/IntegrationTests/utils/validation-utils.tsx	62.50%	3 Missing ⚠️
.../components/IntegrationTests/ContextSelectList.tsx	50.00%	1 Missing ⚠️
...nTests/IntegrationTestForm/IntegrationTestView.tsx	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
- Coverage   80.15%   80.09%   -0.06%     
==========================================
  Files         569      570       +1     
  Lines       21391    21443      +52     
  Branches     5291     5307      +16     
==========================================
+ Hits        17146    17175      +29     
- Misses       4220     4244      +24     
+ Partials       25       24       -1

Flag	Coverage Δ
unittests	`80.09% <83.63%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CryptoRodeo · 2025-01-09T21:28:59Z

Code patch check looks safe to ignore IMO.

sahil143 · 2025-01-13T09:33:06Z

src/components/IntegrationTests/IntegrationTestForm/IntegrationTestView.tsx

-    if (!contexts?.length) return [];
+  const getFormContextValues = (
+    integrateTest: IntegrationTestScenarioKind | null | undefined,
+  ): FormContext[] => {


Adding unit tests for this utility should help with the codecoverage

Good catch! Test added.

sahil143 · 2025-01-13T09:38:42Z

src/components/IntegrationTests/utils/test-utils.ts

@@ -0,0 +1,17 @@
+import { fireEvent, screen, act } from '@testing-library/react';


My only concern is that the testing utilities should not be kept with the actual component utilities. I like the idea of creating separate utilities for opening dropdowns that can be used everywhere else. You can rename the functions to something more specific like openIntegrationTestContextDropown and move them to the common test-utils in src/utils/test-utils.ts. I am open to any other suggestions.

CryptoRodeo · 2025-01-13T18:22:40Z

@sahil143 That's fair. I moved the test utils to src/utils/test-utils.ts and renamed them: https://github.com/konflux-ci/konflux-ui/pull/58/files#diff-f13f3abfe65497faaec0c4ca81fa8037b6aa1094ed9e293becd345375fdcfda6R250-R261

The user should be able to unselect the default context during the creation of an integration test. Other changes: - Contexts are now required (at least one) Signed-off-by: Bryan Ramos <[email protected]>

CryptoRodeo force-pushed the konflux-5886 branch 2 times, most recently from 9d36d95 to eb33e13 Compare December 18, 2024 21:10

CryptoRodeo marked this pull request as ready for review December 18, 2024 21:16

CryptoRodeo self-assigned this Dec 18, 2024

CryptoRodeo force-pushed the konflux-5886 branch from eb33e13 to 8c1f7d9 Compare December 18, 2024 21:38

CryptoRodeo force-pushed the konflux-5886 branch from 8c1f7d9 to fcd4962 Compare December 19, 2024 14:04

CryptoRodeo requested review from JoaoPedroPP, testcara and sahil143 December 19, 2024 15:08

JoaoPedroPP previously approved these changes Jan 6, 2025

View reviewed changes

CryptoRodeo added this pull request to the merge queue Jan 6, 2025

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 6, 2025

sahil143 reviewed Jan 8, 2025

View reviewed changes

CryptoRodeo dismissed JoaoPedroPP’s stale review via a7feb72 January 8, 2025 15:55

CryptoRodeo force-pushed the konflux-5886 branch 3 times, most recently from 9338d33 to 42c716f Compare January 9, 2025 19:49

CryptoRodeo requested a review from sahil143 January 9, 2025 20:56

sahil143 reviewed Jan 13, 2025

View reviewed changes

CryptoRodeo force-pushed the konflux-5886 branch 2 times, most recently from f64d63d to 5aecd12 Compare January 13, 2025 17:03

CryptoRodeo requested a review from sahil143 January 13, 2025 18:23

CryptoRodeo force-pushed the konflux-5886 branch from 5aecd12 to b06bc0f Compare January 14, 2025 21:31

fix(KONFLUX-5886): allow unselecting default context

4611189

The user should be able to unselect the default context during the creation of an integration test. Other changes: - Contexts are now required (at least one) Signed-off-by: Bryan Ramos <[email protected]>

CryptoRodeo force-pushed the konflux-5886 branch from b06bc0f to 4611189 Compare January 15, 2025 14:58

sahil143 approved these changes Jan 15, 2025

View reviewed changes

sahil143 merged commit b5460e5 into konflux-ci:main Jan 15, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(KONFLUX-5886): allow unselecting default context #58

fix(KONFLUX-5886): allow unselecting default context #58

CryptoRodeo commented Dec 18, 2024

CryptoRodeo commented Dec 18, 2024

CryptoRodeo commented Dec 18, 2024

testcara commented Dec 19, 2024

testcara commented Dec 19, 2024

JoaoPedroPP commented Jan 6, 2025

StanislavJochman commented Jan 7, 2025

sahil143 Jan 8, 2025 •

edited

Loading

CryptoRodeo Jan 8, 2025

CryptoRodeo Jan 8, 2025 •

edited

Loading

CryptoRodeo Jan 9, 2025 •

edited

Loading

sahil143 Jan 13, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

CryptoRodeo commented Jan 9, 2025

sahil143 Jan 13, 2025

CryptoRodeo Jan 13, 2025

sahil143 Jan 13, 2025

CryptoRodeo commented Jan 13, 2025

		@@ -0,0 +1,17 @@
		import { fireEvent, screen, act } from '@testing-library/react';

fix(KONFLUX-5886): allow unselecting default context #58

fix(KONFLUX-5886): allow unselecting default context #58

Conversation

CryptoRodeo commented Dec 18, 2024

CryptoRodeo commented Dec 18, 2024

CryptoRodeo commented Dec 18, 2024

testcara commented Dec 19, 2024

testcara commented Dec 19, 2024

JoaoPedroPP commented Jan 6, 2025

StanislavJochman commented Jan 7, 2025

sahil143 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

CryptoRodeo Jan 8, 2025

Choose a reason for hiding this comment

CryptoRodeo Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

CryptoRodeo Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

sahil143 Jan 13, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

CryptoRodeo commented Jan 9, 2025

sahil143 Jan 13, 2025

Choose a reason for hiding this comment

CryptoRodeo Jan 13, 2025

Choose a reason for hiding this comment

sahil143 Jan 13, 2025

Choose a reason for hiding this comment

CryptoRodeo commented Jan 13, 2025

sahil143 Jan 8, 2025 •

edited

Loading

CryptoRodeo Jan 8, 2025 •

edited

Loading

CryptoRodeo Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading