-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: enable basic runner #7835
Conversation
* feat: enable the test result pane * test: bring back tests and cleanups * chore: replace tabitem with tabpanel * chore: useMemo for test result counts * refactor: abstract RequestTestResultRows as a component
results: RequestTestResult[]; | ||
} | ||
|
||
export const sendActionImp = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original sendAction logic are copied to this imp function although there seems lots of changes below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit complext, I would like some explanation here on a call. I think it may be a good idea for us to hop on a call to go over all the PR. I will reserve 1-2 hrs time in my evening. Please let me know when you are available.
import { HelpTooltip } from '../components/help-tooltip'; | ||
import { Icon } from '../components/icon'; | ||
import { Pane, PaneBody, PaneHeader } from '../components/panes/pane'; | ||
// import { RequestTestResultPane } from '../components/panes/request-test-result-pane'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes are commented out to avoid introducing too many changes.
It happens in the latest dev as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this PR without much context. I may need your help to unpack lots of things as the collection runner is complex scope. I suggest we hop on a call to go over each PR in order of your suggestion.
@@ -30,7 +30,7 @@ import { ImportModal } from '../modals/import-modal'; | |||
import { WorkspaceDuplicateModal } from '../modals/workspace-duplicate-modal'; | |||
import { WorkspaceSettingsModal } from '../modals/workspace-settings-modal'; | |||
|
|||
export const WorkspaceDropdown: FC = () => { | |||
export const WorkspaceDropdown: FC<{}> = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FC has the default type for its prop, so I think P = {}
is not necessary here unless we want to actually pass prop type
@@ -37,3 +36,19 @@ export function completeExecutionStep(requestId: string) { | |||
window.webContents.send(`syncTimers.${requestId}`, { executions: executions.get(requestId) }); | |||
} | |||
} | |||
|
|||
export function updateLatestStepName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind explaining a bit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for updating the timing record in place (in the response pane), as originally it just allows appending records.
@@ -149,7 +150,8 @@ export const loader: LoaderFunction = async ({ params }) => { | |||
invariant(activeWorkspaceMeta, 'Workspace meta not found'); | |||
const activeRequestId = activeWorkspaceMeta.activeRequestId; | |||
const activeRequest = activeRequestId ? await models.request.getById(activeRequestId) : null; | |||
if (activeRequest) { | |||
const isDisplayingRunner = request.url.endsWith('/runner') || request.url.endsWith('/runner?test-end'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to understand more on this url pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mainly for triggering refreshing when returning from the react router action.
<WorkspaceDropdown /> | ||
</Breadcrumb> | ||
<Breadcrumb className="flex text-sm truncate select-none items-center justify-self-end ml-auto mr-2.5 gap-2 text-[--color-font] h-full outline-none data-[focused]:outline-none"> | ||
<Icon icon='circle-play' onClick={onRunCollection} className="cursor-pointer" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i see. It navigates to a different page
results: RequestTestResult[]; | ||
} | ||
|
||
export const sendActionImp = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit complext, I would like some explanation here on a call. I think it may be a good idea for us to hop on a call to go over all the PR. I will reserve 1-2 hrs time in my evening. Please let me know when you are available.
const [iterationFilePath, setIterationFilePath] = useState<File | undefined>(undefined); | ||
|
||
const [direction, setDirection] = useState<'horizontal' | 'vertical'>(settings.forceVerticalLayout ? 'vertical' : 'horizontal'); | ||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there another way to do it without useEffect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is copied from another component for consistent layout behavior, probably it could be deduplicated.
And all content of sendActionImp
is copied from the sendAction
for reusing. I can introduce it later.
@marckong Let me schedule a sync at sometime. |
Note: abstracts the send action in order to use different exception handling. |
* feat: add result pane for runner with iteration dividers * feat: enable runner cancellation * refactor: some minor fixes and enhancements * fix: minor fixes
3c7a123
to
359b005
Compare
@ihexxa is this one still needed or we can close it? |
@filfreire It is no longer needed, let me close it. |
Background
This PR enables basic runner by commenting out some dependencies.
Changes
Ref: INS-4197, INS-4222, INS-4223