Feature/add tests#5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Mocha-based test suite (unit + Spectron E2E) and updates the project’s test scripts and runtime setup to support running those tests in CI.
Changes:
- Added multiple new unit test files for Store, Letter, i18n, and LetterStructure plus a test README.
- Updated E2E test bootstrapping (
test/spec.js) and npm scripts to run unit vs E2E tests. - Updated Electron/Electron Builder versions and adjusted BrowserWindow
webPreferencesand Store/i18n initialization behavior.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/build.yml |
Expands workflow triggers; currently contains a YAML indentation bug that will break the workflow. |
package.json |
Adds mocha test scripts and bumps Electron/Electron Builder versions (version/tooling compatibility concerns). |
app/package.json |
Bumps Electron version inside app/, creating a multi-Electron-version setup risk. |
app/package-lock.json |
Large lockfile rewrite and lockfileVersion downgrade (CI/install reliability risk). |
app/main.js |
Sets insecure webPreferences and adds guard for window-all-closed send. |
app/js/MenuTemplate.js |
Sets insecure webPreferences on Settings/About windows. |
app/js/i18n.js |
Wraps electron-store usage in try/catch; introduces a possible locale undefined crash path. |
app/js/Store.js |
Fixes callback this binding; adds sync callback path that can introduce timing bugs. |
app/js/Letter.js |
Exports Letter for unit tests. |
app/js/About.js |
Makes copyright year dynamic (still uses “Github” in UI text). |
app/i18n/en.json |
Updates intro/help strings. |
LICENSE |
Updates copyright year. |
test/spec.js |
Improves Electron path detection and adds skips/logging for E2E stability. |
test/store.test.js |
Adds Store unit tests, but storeHistory tests currently don’t exercise storeHistory(). |
test/letter.test.js |
Adds basic Letter constructor/toWord tests. |
test/letterStructure.test.js |
Adds tests, but they validate mock implementations rather than production code. |
test/i18n.test.js |
Adds language file consistency checks; includes a brittle “80% must differ” rule. |
test/README.md |
Documents test structure and commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe('getAddress logic', function() { | ||
| // Mock implementation based on the actual logic | ||
| function getAddress(tableRow) { | ||
| const fullAddress = []; | ||
| const attributes = tableRow.children || []; | ||
|
|
||
| function isCellEmpty(cell) { | ||
| return typeof getValueOfTableCell(cell) === "undefined" || | ||
| getValueOfTableCell(cell) === ""; | ||
| } | ||
|
|
||
| function getValueOfTableCell(cell) { | ||
| return cell && cell.value ? cell.value : (cell && cell.text ? cell.text : ""); | ||
| } | ||
|
|
||
| if (!isCellEmpty(attributes[0]) || !isCellEmpty(attributes[1]) || | ||
| !isCellEmpty(attributes[2]) || !isCellEmpty(attributes[3])) { | ||
| let _name = getValueOfTableCell(attributes[0]) + " " + | ||
| getValueOfTableCell(attributes[1]) + " " + | ||
| getValueOfTableCell(attributes[2]) + " " + | ||
| getValueOfTableCell(attributes[3]); | ||
| _name = _name.replace(/ /g, " ").trim(); | ||
| if (_name) fullAddress.push(_name); | ||
| } | ||
| if (!isCellEmpty(attributes[4])) { | ||
| fullAddress.push(getValueOfTableCell(attributes[4])); | ||
| } | ||
| if (!isCellEmpty(attributes[5])) { | ||
| fullAddress.push(getValueOfTableCell(attributes[5])); | ||
| } | ||
| if (!isCellEmpty(attributes[6])) { | ||
| fullAddress.push(getValueOfTableCell(attributes[6])); | ||
| } | ||
| if (!isCellEmpty(attributes[7]) || !isCellEmpty(attributes[8])) { | ||
| let _city = getValueOfTableCell(attributes[7]) + " " + | ||
| getValueOfTableCell(attributes[8]); | ||
| _city = _city.trim(); | ||
| if (_city) fullAddress.push(_city); | ||
| } | ||
| if (!isCellEmpty(attributes[9])) { | ||
| fullAddress.push(getValueOfTableCell(attributes[9])); | ||
| } | ||
| if (fullAddress.length === 0) { | ||
| return ["Doppelklicken um Empfänger hinzuzufügen", "", "", ""]; | ||
| } | ||
| return fullAddress; | ||
| } |
There was a problem hiding this comment.
These tests define mock implementations of getAddress/getCurrentContent/colorize inside the test file rather than importing and exercising the production code. As written, they can pass even if LetterStructure.js breaks. Consider exporting the pure helpers from LetterStructure.js (or moving them to a separate module) and testing those exports instead of duplicating the logic here.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| function createWindow () { | ||
| autoUpdater.checkForUpdates(); | ||
| mainWindow = new BrowserWindow({width: 640, height: 480, backgroundColor: "#04C800"}); | ||
| mainWindow = new BrowserWindow({width: 640, height: 480, backgroundColor: "#04C800", webPreferences: {nodeIntegration: true, contextIsolation: false}}); |
There was a problem hiding this comment.
Enabling webPreferences: { nodeIntegration: true, contextIsolation: false } disables key Electron security protections and allows any renderer XSS to escalate to full Node.js/RCE. Prefer nodeIntegration: false, contextIsolation: true, and expose only the required APIs via a preload script + contextBridge.
| mainWindow = new BrowserWindow({width: 640, height: 480, backgroundColor: "#04C800", webPreferences: {nodeIntegration: true, contextIsolation: false}}); | |
| mainWindow = new BrowserWindow({ | |
| width: 640, | |
| height: 480, | |
| backgroundColor: "#04C800", | |
| webPreferences: { | |
| nodeIntegration: false, | |
| contextIsolation: true, | |
| preload: path.join(__dirname, 'preload.js') | |
| } | |
| }); |
| label: i18n("menu.edit.settings"), | ||
| click: function () { | ||
| settingsWindow = new BrowserWindow({width: 800, height: 600, title: "Settings"}); | ||
| settingsWindow = new BrowserWindow({width: 800, height: 600, title: "Settings", webPreferences: {nodeIntegration: true, contextIsolation: false}}); |
There was a problem hiding this comment.
Same concern here: setting nodeIntegration: true and contextIsolation: false for these auxiliary windows weakens renderer isolation and increases the impact of any injected/remote content. Use a secure preload bridge (or keep Node disabled) instead of enabling Node integration in the window.
| settingsWindow = new BrowserWindow({width: 800, height: 600, title: "Settings", webPreferences: {nodeIntegration: true, contextIsolation: false}}); | |
| settingsWindow = new BrowserWindow({width: 800, height: 600, title: "Settings", webPreferences: {nodeIntegration: false, contextIsolation: true}}); |
| describe('Store.storeHistory', function() { | ||
| beforeEach(function(done) { | ||
| store = new Store(function() { | ||
| // Clear history before each test | ||
| store.set('history', []); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should add new entry to history when history exists', function() { | ||
| // Initialize history first | ||
| store.set('history', []); | ||
|
|
||
| const content1 = { | ||
| sender: 'John Doe', | ||
| receiver: 'Jane Smith', | ||
| subject: 'Test 1', | ||
| content: 'Content 1' | ||
| }; | ||
| const content2 = { | ||
| sender: 'Jane Doe', | ||
| receiver: 'John Smith', | ||
| subject: 'Test 2', | ||
| content: 'Content 2' | ||
| }; | ||
|
|
||
| // Manually add to history since storeHistory requires getCurrentContent | ||
| let history = store.get('history') || []; | ||
| if (!store.compareTwoHistories(content1, history[0] || {})) { | ||
| history.unshift(content1); | ||
| store.set('history', history); | ||
| } | ||
|
|
||
| history = store.get('history'); | ||
| if (!store.compareTwoHistories(content2, history[0] || {})) { | ||
| history.unshift(content2); | ||
| store.set('history', history); | ||
| } |
There was a problem hiding this comment.
The newly added tests in this section don’t actually call store.storeHistory(...); they manually reimplement the logic using compareTwoHistories and direct set/get. This won’t catch regressions in storeHistory itself (e.g., its getCurrentContent() call, cloud sync side-effects, etc.). Refactor the test to stub getCurrentContent (or pass explicit currentContent) and exercise store.storeHistory directly.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @@ -30,6 +31,8 @@ var Store = function (callback) { | |||
| ipcRenderer.send('message', i18n("message.dropboxfailed")); | |||
| } | |||
| }) | |||
| } else if (callback) { | |||
| callback.call(this); | |||
There was a problem hiding this comment.
callback is only invoked after a successful Dropbox download/parse. If filesDownload fails (network/auth/etc.), the callback is never called and callers can hang waiting for initialization. Consider invoking the callback on the error path as well (possibly after showing the failure message), or centralize callback completion so it's guaranteed exactly once.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…on code Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…es window Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…y windows Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…ined branch test Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…ndow Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Extract pure helpers from LetterStructure.js and test actual production code
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…Bridge APIs Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Refactor Store.storeHistory tests to exercise the method directly
Align Electron devDependency version in app/package.json and app/package-lock.json with root
Disable nodeIntegration and enable contextIsolation for auxiliary windows with preload bridges
… in test Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
…stener cleanup Agent-Logs-Url: https://github.com/Samuel3/LetterCreator/sessions/8d47fbaa-3b3d-465a-b84b-ac1ba49449c7 Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Samuel3/LetterCreator/sessions/8d47fbaa-3b3d-465a-b84b-ac1ba49449c7 Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Fix: callback not invoked on Dropbox filesDownload error in Store.js
…kage-lock.json conflicts Agent-Logs-Url: https://github.com/Samuel3/LetterCreator/sessions/e4012f10-97cd-47f1-aef5-c14bf1fc407f Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Added tests, updated Electron and fixed bugs