Conversation
…est stack - Unit tests are broken, but there's a pattern to fix them - Much of this work done using Cursor / "auto" - I've done extremely minimal testing of the web app - I have not tested the electron build
|
I promise hands off from my side :) |
| font-size: $icon-height; | ||
| max-height: $icon-height; | ||
| margin: 0 5px 0 5px; | ||
| color: $white; |
There was a problem hiding this comment.
Not sure why, but the header fontawesome icons for links changed color. This fixes that.
| }); | ||
| if (routerInstance === null) { | ||
| routerInstance = createRouter({ | ||
| history: createWebHashHistory(), |
There was a problem hiding this comment.
There are known issues with vue3 router in history mode in Electron. This also aligns with the current state of our web app, so there shouldn't be any breaking changes here in urls / deep linking.
| import Toast, { toastOptions, installToastGlobalProperties } from './plugins/toastification.js'; | ||
|
|
||
| Vue.config.productionTip = false; | ||
| let appProxy; |
There was a problem hiding this comment.
Desktop entry uses appProxy so Electron IPC handlers registered at load time can access $store, $router, $t, $bvModal, $toast when they run asynchronously.
|
|
||
| const getWrapper = (propsData) => shallowMount(TdGraphThreats, { | ||
| localVue, | ||
| const getWrapper = (propsData) => shallowMount(TdGraphThreats, mountOptions(localVue, { |
There was a problem hiding this comment.
There are some API differences between Vue 2 and Vue 3 around the shallowMount. findComponent just becomes find, and there are a few other differences. This test is a pretty good example of how migration will work for many other tests.
| "vue": "^3.4.21", | ||
| "vue-i18n": "^9.14.1", | ||
| "vue-router": "^4.4.5", | ||
| "vue-toastification": "^2.0.0-rc.5", |
There was a problem hiding this comment.
Leaving vue-toastification for compatibility, however we should switch to a different toast library. rc5 is the latest version, and there has been no updates since. https://github.com/Maronato/vue-toastification/releases
There was a problem hiding this comment.
oh my, last release 4.5 years ago - should we raise a separate issue to track the need for replacing vue-toastification ?
There was a problem hiding this comment.
Yes, 100%
I'm keeping a running tally of follow-on items for the complete vue migration. This will be on that list for sure! Once I'm confident that this is in a good place for review, I will create issues for all the follow-ups (i18n, bootstrap, build/runtime compat warnings, removing compat, etc)
| "@vue/eslint-config-standard": "^7.0.0", | ||
| "@vue/test-utils": "^1.3.0", | ||
| "@vue/test-utils": "^2.4.6", | ||
| "@vue/vue2-jest": "^27.0.0", |
There was a problem hiding this comment.
I'm not 100% sure if we need both vue2-jest and vue3-jest, but we can leave for now.
|
I still have a lot of manual testing to do, but I wanted to highlight that the unit tests are going to make this PR scope grow out of control. I don't want to burden anyone with a PR that size - this is already large enough. There are probably ~30-40 more tests that need updating. We can handle this in a few ways, but here's I'm proposing:
Other options may include:
The first 2 options above would still make this PR significantly larger. @jgadsden - do any of these strategies resonate with you, or do you have any other suggestions on how to keep this under control? 😅 |
| throw error; | ||
| } | ||
| }); | ||
| await this.$store.dispatch(LOGOUT); |
There was a problem hiding this comment.
LOGOUT clears provider.selected, and some of the views assume that there will always be a value for that. This was causing a runtime error in the desktop, but I didn't notice it in the web version oddly enough. Doing the await Promise.resolve is a bit of an ugly pattern, but I don't foresee any issues with dispatching the event after the navigation completes. This did fix the bug.
| "@fortawesome/free-brands-svg-icons": "^6.2.0", | ||
| "@fortawesome/free-solid-svg-icons": "^6.2.0", | ||
| "@fortawesome/vue-fontawesome": "^2.0.8", | ||
| "@fortawesome/vue-fontawesome": "^3.1.3", |
There was a problem hiding this comment.
I was trying to keep the scope down, but for whatever reason I could not get vue-fontawesome v2.x to work inside electron. It was working fine in web, but it was completely breaking the ability to render anything in desktop. Just an unhelpful console error with a blank page. Fortunately, this 2-3 upgrade seems pretty low-lift, and the desktop client is now working again.
…ng back to html inputs and binds
.github/workflows/pull_request.yaml
Outdated
|
|
||
| - name: Run unit tests | ||
| run: npm run test:unit | ||
| # TODO: Reinstate tests as part of build |
There was a problem hiding this comment.
@jgadsden - Do we want to leave this disabled to keep this PR somewhat reasonable in size?
I'm fine with fixing all unit tests as part of this PR, but the size will grow a lot. I want to make sure you can actually review when you have the time!
There was a problem hiding this comment.
good point @lreading , we can reinstate when late it is convenient
| md="6" | ||
| > | ||
| <b-form-group label-cols="auto" id="outofscope-group"> | ||
| <b-form-checkbox id="outofscope" v-model="cellRef.data.outOfScope" @change="onChangeScope()">{{ |
There was a problem hiding this comment.
All b-form-checkbox components had a runtime error, complaining that "assign" is not a function. This was happening in the Bootstrap code, not ours. I couldn't find a ton of information online, but applying the correct classes and constructing these using divs and inputs retains the same style and behavior.
Alternatively, we could create a td-checkbox component, but I think it's overkill, at least for now.
| .contains('Web Application Config (Store)') | ||
| .should('be.visible'); | ||
|
|
||
| cy.get('#show_outofscope').click({ force: true }); |
There was a problem hiding this comment.
Because we're using inputs now, the correct semantics are check and uncheck - click was no longer working the way we expected.
|
I'll be completing #1275 prior to merging this. Having additional e2e coverage for desktop will reduce the risk of regressions when doing breaking dependency upgrades. |
| @@ -0,0 +1,61 @@ | |||
| import * as actual from 'vue-i18n/dist/vue-i18n.cjs'; | |||
There was a problem hiding this comment.
Rather than updating or disabling tens of tests, we can add a temporary compat layer that helps the tests pass assuming the vue2 APIs.
| @@ -0,0 +1,768 @@ | |||
| import * as actual from '@vue/test-utils/dist/vue-test-utils.cjs.js'; | |||
There was a problem hiding this comment.
This file is a bit of a disaster. Needs careful consideration if we want to continue with this pattern or not.
By adding this compat layer, we do not need to update every test that was using the vue2 test utils. Without this layer, we either disable a bunch of tests, or update all of the tests in this PR.
Now whether having this compat layer makes the PR easier or harder to review is 💯% up for discussion....
|
|
||
| it('is the default path', () => { | ||
| expect(homeRoute.path).toEqual(''); | ||
| expect(homeRoute.path).toEqual('/'); |
There was a problem hiding this comment.
Best I can understand, this is just a semantic difference in the new vue router version.
| @@ -0,0 +1,10 @@ | |||
| import { createLocalVue as compatCreateLocalVue } from '@vue/test-utils'; | |||
There was a problem hiding this comment.
Another shim for the 2-3 migration to avoid having to update as many tests.
|
@jgadsden - Thinking about unit tests more: I tried to make a compat layer for unit testing. It's big, ugly, difficult to review, and messy. However, it does allow us to keep the unit tests mostly in tact, and keep passing tests as a CI gate. I did test the compat layer a bit by intentionally introducing regressions to make sure the tests started failing. This was only a spot-check though. The other options I considered:
Each one has its own tradeoffs. My personal preference is this compat layer - however, I do understand that it is adding to the burden of review.. This is already a pretty beefy PR. Do you have a strong preference here? I'm happy to revert the compat layer or explore other options! I think this is finally ready for review. 🤞 I did not thoroughly test any providers outside of the local session. |
|
Thanks @lreading , a huge task (but not thankless!) |
|
Agreed! No rush on this, and I invite anyone in the community who is following along to pull and test these changes as well. I will leave a comment on the issue and make a short post in our Slack channel. 😄 |
Summary:
This upgrades Threat Dragon to Vue 3 using
vue-compat, with the goal of making the smallest set of changes needed to keep the app working and releasable while we continue paying down migration debt in follow-up work.This is still a transitional PR. There is temporary compat code here, especially around Jest and a few Vue 2 APIs. I do not consider that the final state. The goal of this PR is to get the app running correctly on Vue 3 first, then continue cleanup in smaller follow-ons.
Closes #444
Current State
What I Tested
Unit tests: all passing, 2 newly skipped
e2e tests: all passing
Desktop e2e tests: all passing
Visual regression: I had AI help me set up some playwright / visual regression tests to help identify any significant drifts in the UI. I compared 2.6.0 from the demo site to my local, as well as the 2.6.0 published AppImage against a local build. The only notable differences remaining include some minor margin/padding drift that does not materially change the layout or UX.
Help Wanted
I would especially appreciate help testing these areas:
Known Tradeoffs
Description for the changelog:
Upgrade to Vue3
Declaration:
Thanks for submitting a pull request, please make sure:
Other info:
When auditing production dependencies, there are now 0 vulnerabilities!

I think this is ready for review? 🤞