Update tests for slower load of async route components - #1770
matthew-white wants to merge 2 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
d7cbe4a to
c813fb7
Compare
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
Adding some questions and comments about the code.
| return m; | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| if (import.meta.env.NODE_ENV === 'development') console.error(error); |
There was a problem hiding this comment.
I understand the finally below: it's needed to decrement loadingCount. But I don't remember the point of this catch. Why is it useful to console.error() the error if we're also re-throwing it? Perhaps #1743 means that this part is unnecessary now.
Also, the use of import.meta.env.NODE_ENV seems a little unusual to me. What is NODE_ENV in the context of testing? Is it 'development'? I assume so, because if not, this line seems kind of irrelevant to the rest of the work of the commit in which it was added.
In most of the app, we use buildMode instead of something like NODE_ENV. I guess buildMode isn't available in the context of this file though, since it doesn't have access to the Frontend container object.
Another thing I'm wondering: why NODE_ENV instead of MODE?
| console.error(`Component ${asyncRoute.componentName} was not found in the loadAsync() cache.`); | ||
| // eslint-disable-next-line no-console | ||
| console.error('The loadAsync() cache contains', loadAsyncCache.size, 'entries.'); | ||
| throw new Error('component not found in loadAsync() cache'); |
There was a problem hiding this comment.
I don't think this error-logging existed back when I was working on Vitest: when rebasing these commits, I got a merge conflict in this area of the code. I think it makes sense to remove this error-logging, since loadAsyncCache is removed as part of this PR. However, I've added a little more error handling to loadAsync() in src/util/load-async.js.
| const getLoader = (name) => { | ||
| if (!loaders.has(name)) throw new Error(`loader not found for ${name}`); | ||
| return loaders.get(name); | ||
| }; |
There was a problem hiding this comment.
This part wasn't part of the original commit. I just added it when I rebased now. I added it because I removed some error-logging in test/util/http.js as part of resolving a merge conflict.
| const wrapper = lifecycleMount(component, { ...options, container }); | ||
| const mount = async () => { | ||
| const loadedComponent = typeof component === 'string' | ||
| ? (await loadAsync(component)()).default |
There was a problem hiding this comment.
Instead of calling loadAsync() ahead of time in test/index.js (via loadAsyncRouteComponents()), we now do so right before the component definition is needed (before we need to pass it to Vue Test Utils' mount()).
| // Wait for any router navigation to finish. | ||
| if (router != null) await wait(); | ||
| await waitUntil(() => !loadingAnyAsync()); |
There was a problem hiding this comment.
Here, I think we're waiting for two things. That's why there's both wait() + waitUntil():
- Wait for Vue Router navigation hooks to complete. Those should be processed as microtasks, so
wait()should give them enough time to complete. - Wait for async route components to load (now longer than a microtask).
matthew-white
changed the title
|
I have a third commit locally, but when I tried to rebase, there were a lot of merge conflicts. The commit message is "Update router tests now that it takes longer to import async components", so it seems relevant to these other two commits. The commit only changes test/router.spec.js. I don't really remember this commit, but as I look at it now, my guess as to the problem the commit is trying to solve is that router redirects can now happen before the async route component is loaded. Before, the async route component would be loaded and would send requests before the redirect would happen. Now, the redirect happens before the async component is loaded, so the component never sends requests. But I'm having trouble rebasing the commit, but here's the diff. I'm noticing that some of the route paths in the diff no longer exist: those pages have been removed or merged into other pages. That's probably part of the reason for the merge conflicts. Diffdiff --git a/test/router.spec.js b/test/router.spec.js
index c6962d1c3..3cc89c75d 100644
--- a/test/router.spec.js
+++ b/test/router.spec.js
@@ -562,28 +562,27 @@ describe('createCentralRouter()', () => {
});
it('redirects the user from .../users', () =>
- load('/projects/1/users', {}, {
- projectAssignments: () => mockResponse.problem(403.1)
- })
+ load('/projects/1', {}, { deletedForms: false })
+ .complete()
+ .route('/projects/1/users')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../app-users', () =>
- load('/projects/1/app-users', {}, {
- fieldKeys: () => mockResponse.problem(403.1)
- })
+ load('/projects/1', {}, { deletedForms: false })
+ .complete()
+ .route('/projects/1/app-users')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../form-access', () =>
- load('/projects/1/form-access', {}, {
- fieldKeys: () => mockResponse.problem(403.1),
- formSummaryAssignments: () => mockResponse.problem(403.1)
- })
+ load('/projects/1', {}, { deletedForms: false })
+ .complete()
+ .route('/projects/1/form-access')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -603,8 +602,15 @@ describe('createCentralRouter()', () => {
testData.standardFormAttachments.createPast(1);
});
+ it('does not redirect the user from .../submissions', async () => {
+ const app = await load('/projects/1/forms/f/submissions');
+ app.vm.$route.path.should.equal('/projects/1/forms/f/submissions');
+ });
+
it('redirects the user from the form overview', () =>
- load('/projects/1/forms/f')
+ load('/projects/1/forms/f/submissions')
+ .complete()
+ .route('/projects/1/forms/f')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -615,34 +621,37 @@ describe('createCentralRouter()', () => {
app.vm.$route.path.should.equal('/projects/1/forms/f/versions');
});
- it('does not redirect the user from .../submissions', async () => {
- const app = await load('/projects/1/forms/f/submissions');
- app.vm.$route.path.should.equal('/projects/1/forms/f/submissions');
- });
-
it('redirects the user from .../public-links', () =>
- load('/projects/1/forms/f/public-links')
+ load('/projects/1/forms/f/submissions')
+ .complete()
+ .route('/projects/1/forms/f/public-links')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../settings', () =>
- load('/projects/1/forms/f/settings')
+ load('/projects/1/forms/f/submissions')
+ .complete()
+ .route('/projects/1/forms/f/settings')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../draft', () =>
- load('/projects/1/forms/f/draft')
+ load('/projects/1/forms/f/submissions')
+ .complete()
+ .route('/projects/1/forms/f/draft')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../draft/attachments', () =>
- load('/projects/1/forms/f/draft/attachments')
+ load('/projects/1/forms/f/submissions')
+ .complete()
+ .route('/projects/1/forms/f/draft/attachments')
.respondFor('/', { users: false })
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -766,7 +775,12 @@ describe('createCentralRouter()', () => {
describe('form overview', () => {
it('redirects a user whose first navigation is to the route', () =>
- load('/projects/1/forms/f')
+ load('/projects/1/forms/f', {}, {
+ // Responses will be returned to FormShow before FormOverview even
+ // loads. That means that FormOverview won't have a chance to send
+ // a request for publishedAttachments before the user is redirected.
+ publishedAttachments: false
+ })
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -788,7 +802,10 @@ describe('createCentralRouter()', () => {
attachments: () => mockResponse.problem(404.1)
})
.complete()
- .load('/projects/1/forms/f', { project: false })
+ .load('/projects/1/forms/f', {
+ project: false,
+ publishedAttachments: false
+ })
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -796,28 +813,36 @@ describe('createCentralRouter()', () => {
});
it('redirects the user from .../versions', () =>
- load('/projects/1/forms/f/versions', {}, { formVersions: () => [] })
+ load('/projects/1/forms/f/draft')
+ .complete()
+ .route('/projects/1/forms/f/versions')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../submissions', () =>
- load('/projects/1/forms/f/submissions')
+ load('/projects/1/forms/f/draft')
+ .complete()
+ .route('/projects/1/forms/f/submissions')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../public-links', () =>
- load('/projects/1/forms/f/public-links')
+ load('/projects/1/forms/f/draft')
+ .complete()
+ .route('/projects/1/forms/f/public-links')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../settings', () =>
- load('/projects/1/forms/f/settings')
+ load('/projects/1/forms/f/draft')
+ .complete()
+ .route('/projects/1/forms/f/settings')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -835,7 +860,12 @@ describe('createCentralRouter()', () => {
describe('.../draft', () => {
it('redirects a user whose first navigation is to the route', () =>
- load('/projects/1/forms/f/draft')
+ load('/projects/1/forms/f/draft', {}, {
+ // Responses will be returned to FormShow before FormDraftStatus
+ // even loads. That means that FormDraftStatus won't have a chance
+ // to send a request for formVersions before the user is redirected.
+ formVersions: false
+ })
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -858,7 +888,10 @@ describe('createCentralRouter()', () => {
formVersions: () => []
})
.complete()
- .load('/projects/1/forms/f/draft', { project: false })
+ .load('/projects/1/forms/f/draft', {
+ project: false,
+ formVersions: false
+ })
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -866,14 +899,18 @@ describe('createCentralRouter()', () => {
});
it('redirects the user from .../draft/attachments', () =>
- load('/projects/1/forms/f/draft/attachments')
+ load('/projects/1/forms/f')
+ .complete()
+ .route('/projects/1/forms/f/draft/attachments')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
}));
it('redirects the user from .../draft/testing', () =>
- load('/projects/1/forms/f/draft/testing')
+ load('/projects/1/forms/f')
+ .complete()
+ .route('/projects/1/forms/f/draft/testing')
.respondFor('/')
.afterResponses(app => {
app.vm.$route.path.should.equal('/');
@@ -895,7 +932,7 @@ describe('createCentralRouter()', () => {
form: () => testData.extendedForms.first(),
formDraft: () => testData.extendedFormDrafts.first(),
attachments: () => mockResponse.problem(404.1),
- formVersions: () => []
+ formVersions: false
})
.respondFor('/')
.afterResponses(app => { |
|
I'm hoping to return to this change later in v2026.3 after entity upload is merged, or perhaps early in v2026.4. I feel like it's an overall improvement, even if we weren't wanting to move to Vitest. |
This branch has not been deployed
1 participant
When I was working on Vitest (getodk/central#1267), one problem I ran into is that
mockHttp()assumes that async route components can be loaded almost instantaneously:loadAsyncRouteComponents()(test/util/load-async.js)import()of a route component (vialoadAsync()), the async route component has already been loaded.import()was extremely fast. As long as async route components were preloaded, waiting a single tick ofsetTimeout()(viawait()from test/util/util.js) was enough to ensure thatimport()had time to complete.import()took too long (longer than a tick ofsetTimeout()), so things broke.mockHttp()so that it no longer assumes thatimport()is near-instantaneous (the first commit). With that change,loadAsyncRouteComponents()is also no longer needed (the second commit).Why is this the best possible solution? Were any other approaches considered?
I haven't looked at these commits in a long time, and I'm marking this PR as draft. There may be aspects of this code that I'd want to adjust before merging. I'll add code comments explaining particular lines.
What has been done to verify that this works as intended?
CI isn't passing, so this PR still seems to need a little work. In terms of verification, the main thing I'd want to see is that tests (specifically
mockHttp()) continue to work.