diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d9484e5c64..e80d69d2df 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -89,6 +89,7 @@ jobs: npm-version: [10.x] server-version: ['32'] app-version: ['stable'] + seed: ["${{ github.sha }}"] floccus-adapter: - fake - nextcloud-bookmarks @@ -117,6 +118,7 @@ jobs: browsers: firefox node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} - app-version: master server-version: 32 floccus-adapter: nextcloud-bookmarks @@ -124,6 +126,7 @@ jobs: browsers: firefox node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} - app-version: master server-version: 32 floccus-adapter: nextcloud-bookmarks @@ -131,6 +134,7 @@ jobs: browsers: chrome node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} - app-version: stable server-version: 32 floccus-adapter: fake-noCache @@ -138,6 +142,7 @@ jobs: browsers: firefox node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} - app-version: master server-version: 32 floccus-adapter: fake @@ -145,6 +150,7 @@ jobs: browsers: firefox node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} - app-version: master server-version: 32 floccus-adapter: fake @@ -152,8 +158,49 @@ jobs: browsers: chrome node-version: 14.x npm-version: 7.x + seed: ${{ github.sha }} + - app-version: master + server-version: 32 + floccus-adapter: fake + test-name: benchmark root + browsers: chrome + node-version: 14.x + npm-version: 7.x + seed: "3" + - app-version: master + server-version: 32 + floccus-adapter: fake + test-name: benchmark root + browsers: chrome + node-version: 14.x + npm-version: 7.x + seed: "6" + - app-version: master + server-version: 32 + floccus-adapter: fake + test-name: benchmark root + browsers: chrome + node-version: 14.x + npm-version: 7.x + seed: "0.5202964461460992" + - app-version: master + server-version: 32 + floccus-adapter: fake + test-name: benchmark root + browsers: chrome + node-version: 14.x + npm-version: 7.x + seed: "12dd05da349df3d71c9e48e764f5d50ec2bbc88d" + - app-version: master + server-version: 32 + floccus-adapter: fake + test-name: benchmark root + browsers: chrome + node-version: 14.x + npm-version: 7.x + seed: "cfc00c4b69afec232e3b26244c2bc98ed6d547b6" - name: ${{ matrix.browsers == 'firefox' && '🦊' || '🔵' }} ${{matrix.floccus-adapter}}:${{ matrix.test-name}} ⭐${{ matrix.app-version }} + name: ${{ matrix.browsers == 'firefox' && '🦊' || '🔵' }} ${{matrix.floccus-adapter}}:${{ matrix.test-name}} ⭐${{ matrix.app-version }} ${{ matrix.test-name == 'benchmark root' && matrix.seed != github.sha && '🌱' || ''}}${{ matrix.test-name == 'benchmark root' && matrix.seed != github.sha && matrix.seed || ''}} services: hub: @@ -272,7 +319,7 @@ jobs: env: SELENIUM_BROWSER: ${{ matrix.browsers }} FLOCCUS_TEST: ${{matrix.floccus-adapter}} ${{ matrix.test-name}} - FLOCCUS_TEST_SEED: ${{ github.sha }} + FLOCCUS_TEST_SEED: ${{matrix.seed}} TEST_HOST: 172.17.0.1:8174 run: | npm run test @@ -283,7 +330,7 @@ jobs: env: SELENIUM_BROWSER: ${{ matrix.browsers }} FLOCCUS_TEST: ${{matrix.floccus-adapter}} ${{ matrix.test-name}} - FLOCCUS_TEST_SEED: ${{ github.sha }} + FLOCCUS_TEST_SEED: ${{matrix.seed}} GIST_TOKEN: ${{ secrets.GIST_TOKEN }} GOOGLE_API_REFRESH_TOKEN: ${{ secrets.GOOGLE_API_REFRESH_TOKEN }} DROPBOX_API_REFRESH_TOKEN: ${{ secrets.DROPBOX_API_REFRESH_TOKEN }} diff --git a/src/lib/strategies/Default.ts b/src/lib/strategies/Default.ts index b3061a8d73..38aee796bc 100644 --- a/src/lib/strategies/Default.ts +++ b/src/lib/strategies/Default.ts @@ -776,12 +776,17 @@ export default class SyncProcess { this.hashSettings, false ) - await subScanner.run() + const scanResult = await subScanner.run() newMappings.push([concurrentCreation.payload, action.payload.id]) await Parallel.each(newMappings, async([oldItem, newId]) => { await this.addMapping(action.payload.location === ItemLocation.LOCAL ? this.localTree : this.server, oldItem, newId) },1) - // TODO: subScanner may contain residual CREATE/REMOVE actions that need to be added to mappings + + // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here + // We do not act on REMOVEs, only on CREATEs as we merge the two sides + scanResult.CREATE.getActions().forEach(action => + targetPlan.CREATE.commit({type: action.type, payload: action.payload}) + ) return } diff --git a/src/lib/strategies/Merge.ts b/src/lib/strategies/Merge.ts index 63db482d9b..14f8e5a49a 100644 --- a/src/lib/strategies/Merge.ts +++ b/src/lib/strategies/Merge.ts @@ -152,11 +152,11 @@ export default class MergeSyncProcess extends DefaultSyncProcess { this.hashSettings, false ) - await subScanner.run() + const scanResult = await subScanner.run() newMappings.push([concurrentCreation.payload, action.payload.id]) await Parallel.each( newMappings, - async([oldItem, newId]) => { + async ([oldItem, newId]) => { await this.addMapping( action.payload.location === ItemLocation.LOCAL ? this.localTree @@ -167,7 +167,16 @@ export default class MergeSyncProcess extends DefaultSyncProcess { }, 1 ) - // TODO: subScanner may contain residual CREATE/REMOVE actions that need to be added to mappings + + // SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here + // We do not act on REMOVEs, only on CREATEs as we merge the two sides + scanResult.CREATE.getActions().forEach((action) => + targetPlan.CREATE.commit({ + type: action.type, + payload: action.payload, + }) + ) + return } diff --git a/src/test/test.js b/src/test/test.js index 5755bea3c2..20230f488d 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -4476,6 +4476,77 @@ describe('Floccus', function() { false ) }) + it('should keep residual creates when merging concurrently created folders', async function() { + const localRoot1 = account1.getData().localRoot + const localRoot2 = account2.getData().localRoot + + await browser.bookmarks.create({ + title: 'unrelated', + parentId: localRoot1 + }) + + await account1.sync() + expect(account1.getData().error).to.not.be.ok + await account2.sync() + expect(account2.getData().error).to.not.be.ok + + const folder1 = await browser.bookmarks.create({ + title: 'shared', + parentId: localRoot1 + }) + await browser.bookmarks.create({ + title: 'from account 1', + url: 'https://account1.example/', + parentId: folder1.id + }) + + const folder2 = await browser.bookmarks.create({ + title: 'shared', + parentId: localRoot2 + }) + await browser.bookmarks.create({ + title: 'from account 2', + url: 'https://account2.example/', + parentId: folder2.id + }) + + await account1.sync() + expect(account1.getData().error).to.not.be.ok + await account2.sync() + expect(account2.getData().error).to.not.be.ok + await account1.sync() + expect(account1.getData().error).to.not.be.ok + + const serverTree = await getAllBookmarks(account1) + expectTreeEqual( + serverTree, + new Folder({ + title: serverTree.title, + children: [ + new Folder({ + title: 'unrelated', + children: [], + }), + new Folder({ + title: 'shared', + children: [ + new Bookmark({ title: 'from account 1', url: 'https://account1.example/' }), + new Bookmark({ title: 'from account 2', url: 'https://account2.example/' }), + ] + }) + ] + }), + false, + false + ) + + const tree1 = await account1.localTree.getBookmarksTree(true) + const tree2 = await account2.localTree.getBookmarksTree(true) + tree1.title = serverTree.title + tree2.title = serverTree.title + expectTreeEqual(tree1, serverTree, false, false) + expectTreeEqual(tree2, serverTree, false, false) + }) it('should handle concurrent hierarchy reversals', async function() { const localRoot = account1.getData().localRoot const aFolder = await browser.bookmarks.create({