-
Notifications
You must be signed in to change notification settings - Fork 3.3k
internal: (studio) fix studio and runner states during opening, closing, and refreshing #32153
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
Conversation
cypress
|
Project |
cypress
|
Branch Review |
fix-studio-reload
|
Run status |
|
Run duration | 19m 34s |
Commit |
|
Committer | Adam Stone-Lord |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
13
|
|
1101
|
|
0
|
|
26537
|
View all changes introduced in this branch ↗︎ |
UI Coverage
44.99%
|
|
---|---|
|
187
|
|
157
|
Accessibility
97.72%
|
|
---|---|
|
4 critical
8 serious
2 moderate
2 minor
|
|
110
|
@@ -862,7 +867,7 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
}, | |||
|
|||
needsUrl: (state) => { | |||
return state.isActive && !state.url && !state.isFailed | |||
return state.isActive && !state.url && !state.isFailed && state._hasStarted |
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 was causing the AUT URL to get removed whenever the panel was open at all, even if it was just the new test screen
// Only intercept logs when Studio is actually recording a specific test | ||
// Don't intercept when Studio is just open in "new test" mode | ||
if (this.studioStore.isActive && this.studioStore.testId) { |
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 this function still needed? We don't even display logs anymore.
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.
yeah this still gets called - we don't display anything but it seems like if the logs are intercepted here, they don't get displayed in the command log. So I need to update this because otherwise when we refresh the page and studio is open to "new test", the test body section doesn't get displayed in the left panel.
@@ -175,6 +175,11 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
this.newTestLineNumber = undefined | |||
}, | |||
|
|||
needsProtocolCleanup () { | |||
// Protocol cleanup (page reload) is only needed if Studio has actually been used for recording | |||
return this._hasStarted || this.logs.length > 0 || this._isStudioCreatedTest |
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 the check on logs
needed? We don't display studio logs anymore.
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.
yeah that condition isn't needed... I changed it to this.testId
@@ -175,6 +175,11 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
this.newTestLineNumber = undefined | |||
}, | |||
|
|||
needsProtocolCleanup () { | |||
// Protocol cleanup (page reload) is only needed if Studio has actually been used for recording | |||
return this._hasStarted || this.logs.length > 0 || this._isStudioCreatedTest |
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 think we would want to cleanup protocol regardless of whether or not an action has been recorded since protocol would'e still been initialized and possibly written to. I'm thinking we may just need to check if there is a testId
.
@@ -857,7 +864,9 @@ export class EventManager { | |||
performance.measure('run', 'run-s', 'run-e') | |||
}) | |||
|
|||
const hasRunnableId = !!this.studioStore.testId || !!this.studioStore.suiteId | |||
const hasActiveStudio = !!this.studioStore.testId || | |||
(!!this.studioStore.suiteId && this.studioStore._hasStarted) || |
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'm not sure this check is needed.
(!!this.studioStore.suiteId && this.studioStore._hasStarted) ||
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.
yeah good call, it isn't. I removed those two conditions from this variable
// verify URL still shows suite mode, not edit test mode | ||
cy.location().its('hash').should('contain', 'suiteId=r1').and('not.contain', 'testId=') | ||
|
||
cy.waitForSpecToFinish() |
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.
Why do we need to waitForSpecToFinish
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.
We do not - removed
cy.get('.test').should('have.length', 1) | ||
cy.get('.test').first().should('have.class', 'runnable-active') | ||
|
||
cy.wait(500) |
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 we can wait instead of a hard-coded value? Seems like may we should be using waitForSpecToFinish
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.
I ended up not needing this. I originally added it because the test wasn't correctly failing when I backed out my code changes - but I just tested it again and everything works without waiting
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.
- AUT URL getting replaced with "Enter URL" when tests are running and the "new test" view is open in the studio panel
- The spec re-runs when the user closes the studio panel from the "new test" view
We should add a couple tests to verify this new behavior.
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.
added tests for both of these 👍🏻
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.
Added a few comments.
@@ -737,6 +737,27 @@ describe('studio functionality', () => { | |||
cy.location().its('hash').and('not.contain', 'testId=').and('not.contain', 'studio=') | |||
}) | |||
|
|||
it('does not prompt for URL when creating a new test in studio', () => { |
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('does not prompt for URL when creating a new test in studio', () => { | |
it('does not prompt for URL when creating a new test in studio that already visits', () => { |
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 think I need to re-word this a different way. The issue was that when the user opens the studio panel to "new test" mode, we were already prompting for a URL in the address bar, even when the spec file was currently running and the AUT was at a URL already.
We only want to prompt for a URL once the test is actually created and it doesn't visit anything yet
@@ -175,6 +175,11 @@ export const useStudioStore = defineStore('studioRecorder', { | |||
this.newTestLineNumber = undefined | |||
}, | |||
|
|||
needsProtocolCleanup () { | |||
// Protocol cleanup (page reload) is only needed if Studio has actually been used for recording |
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 don't think this comment is accurate anymore.
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 think it is... we only want to refresh the page when studio closes if we've actually entered recording mode (like, we've made it past the "new test" screen and actually created a test or opened an existing test in studio)
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.
ok, so by recording
you just mean entering single test mode not that the user actually recorded anything?
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.
yeah, exactly - I'll update the comment to make that more clear
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
This PR fixes four main issues:
Steps to test
Test out the three issues that I mentioned above ^
PR Tasks
cypress-documentation
?type definitions
?