[12428] CustomService is run twice#12430
Merged
christian-bromann merged 45 commits intowebdriverio:mainfrom Apr 24, 2024
Merged
[12428] CustomService is run twice#12430christian-bromann merged 45 commits intowebdriverio:mainfrom
christian-bromann merged 45 commits intowebdriverio:mainfrom
Conversation
|
|
ac61b12 to
512088e
Compare
Member
|
@jemishgopani thanks for the patch, mind taking a look at the failing smoke tests? I think your solution fails for when you pass in command line arguments e.g. |
…gopani/webdriverio into fix/duplicate-service-run
Contributor
Author
|
@christian-bromann I have tried to fix please take a look and provide your feedback. |
christian-bromann
requested changes
Mar 6, 2024
…gopani/webdriverio into fix/duplicate-service-run
jemishgopani
commented
Mar 9, 2024
Member
christian-bromann
left a comment
There was a problem hiding this comment.
@jemishgopani can we merge what happens in mergeReportersAndServices into the custom merge function? I think there is no need to have an extra function to merge reporters and services, instead we can just use the custom merger
Contributor
Author
|
Hi @christian-bromann |
Member
christian-bromann
left a comment
There was a problem hiding this comment.
@jemishgopani thanks for adding a test. I reviewed the changes and it doesn't seem to fix the overall issue. Let me explain:
- I tried to verify whether removing your changes in
packages/wdio-config/src/node/ConfigParser.tswill make the test break, unfortunately it didn't - then I tried to verify that the issue is reproducible in the unit test, which it is, for that I applied this change:
diff --git a/packages/wdio-config/tests/node/configparser.test.ts b/packages/wdio-config/tests/node/configparser.test.ts
index d2ff81ce0..33f170fb4 100644
--- a/packages/wdio-config/tests/node/configparser.test.ts
+++ b/packages/wdio-config/tests/node/configparser.test.ts
@@ -13,7 +13,6 @@ import MockFileContentBuilder from '../lib/MockFileContentBuilder.js'
import type { FilePathsAndContents, MockSystemFilePath, MockSystemFolderPath } from '../lib/MockPathService.js'
import ConfigParserBuilder from '../lib/ConfigParserBuilder.js'
import { FileNamed, realReadFilePair, realRequiredFilePair } from '../lib/FileNamed.js'
-import TestCustomService from '../__fixtures__/test-custom-service.js'
const log = logger('')
const __dirname = url.fileURLToPath(new URL('.', import.meta.url))
@@ -41,6 +40,8 @@ vi.mock('@babel/register', () => ({
default: vi.fn()
}))
+class CustomService {}
+
/**
* Entirely in memory config structure to avoid reading the file system at all
*
@@ -70,7 +71,8 @@ const TestWdioConfig_AllInMemory = (): MockFileContent => ({
path.join(root, '/tests/validateConfig.test.ts'),
path.join(root, '/tests/..', 'src/index.ts')
]
- }
+ },
+ services: [[CustomService, { foo: 'bar' }]],
}
})
@@ -691,26 +693,24 @@ describe('ConfigParser', () => {
expect(configParser.getCapabilities()).toMatchObject([{ browserName: 'safari' }])
})
- it('should allow to specifying services', async () => {
+ it.only('should allow to specifying services', async () => {
const configParser = await ConfigParserForTest(FIXTURES_CONF)
await configParser.initialize({
services: [
- [TestCustomService, { timestamp: 1709633731716 }]
+ [CustomService, { foo: 'bar' }]
]
})
- const services = (await configParser.getConfig())['services']
+ const services = configParser.getConfig().services
expect(services).toHaveLength(1)
expect(services[0][1].timestamp).toBe(1709633731716)
- expect(typeof services[0][0]).toBe(typeof TestCustomService)
+ expect(typeof services[0][0]).toBe(typeof CustomService)
})
it('should allow to specifying reporters', async () => {
const configParser = await ConfigParserForTest(FIXTURES_CONF)
await configParser.initialize({
- reporters: [
- ['dot', 'spec']
- ]
+ reporters: ['dot', 'spec']
})
const reporters = (await configParser.getConfig())['reporters']Note: the unit tests load config files from memory and not from the actual file system. So there isn't a need for test-custom-service.js.
I would suggest creating the custom merge as following: merge the service and reporter arrays by removing all entry objects from one side, e.g.:
diff --git a/packages/wdio-config/src/node/ConfigParser.ts b/packages/wdio-config/src/node/ConfigParser.ts
index f828b3df3..f4f6302d9 100644
--- a/packages/wdio-config/src/node/ConfigParser.ts
+++ b/packages/wdio-config/src/node/ConfigParser.ts
@@ -2,7 +2,7 @@ import path from 'node:path'
import logger from '@wdio/logger'
import { deepmerge, deepmergeCustom } from 'deepmerge-ts'
-import type { Capabilities, Options, Services } from '@wdio/types'
+import type { Capabilities, Options, Reporters, Services } from '@wdio/types'
import RequireLibrary from './RequireLibrary.js'
import FileSystemPathService from './FileSystemPathService.js'
@@ -13,6 +13,8 @@ import { SUPPORTED_HOOKS, SUPPORTED_FILE_EXTENSIONS, DEFAULT_CONFIGS, NO_NAMED_C
import type { PathService, ModuleImportService } from '../types.js'
const log = logger('@wdio/config:ConfigParser')
+const MERGE_DUPLICATION = ['services', 'reporters'] as const
+type KeyWithMergeDuplication = (typeof MERGE_DUPLICATION)[number]
type Spec = string | string[]
type ESMImport = { config?: TestrunnerOptionsWithParameters }
@@ -182,12 +184,11 @@ export default class ConfigParser {
* Add deepmergeCustom to remove array('services', 'reporters', 'capabilities') duplication in the config object
*/
const customDeepMerge = deepmergeCustom({
- mergeArrays: (values, utils, meta) => {
- if (meta && ['services', 'reporters', 'capabilities'].includes(meta.key as string)) {
- const mergedArr = deepmerge(this._config[meta.key as keyof WebdriverIO.Config], object[meta.key as keyof WebdriverIO.Config]) as []
- return mergedArr.filter((item, index, merged) => {
- return merged.indexOf(item) === index
- })
+ mergeArrays: ([oldValue, newValue], utils, meta) => {
+ const key = meta?.key as KeyWithMergeDuplication
+ if (meta && MERGE_DUPLICATION.includes(key)) {
+ const origWithoutObjectEntries = oldValue.filter((value: [Services.ServiceClass, WebdriverIO.ServiceOption] | [Reporters.ReporterClass, WebdriverIO.ReporterOption]) => typeof value[0] === 'object')
+ return new Set(deepmerge(newValue, origWithoutObjectEntries))
}
return utils.actions.defaultMerge
}
diff --git a/packages/wdio-config/tests/node/configparser.test.ts b/packages/wdio-config/tests/node/configparser.test.ts
index d2ff81ce0..d22c9ad04 100644
--- a/packages/wdio-config/tests/node/configparser.test.ts
+++ b/packages/wdio-config/tests/node/configparser.test.ts
@@ -13,7 +13,6 @@ import MockFileContentBuilder from '../lib/MockFileContentBuilder.js'
import type { FilePathsAndContents, MockSystemFilePath, MockSystemFolderPath } from '../lib/MockPathService.js'
import ConfigParserBuilder from '../lib/ConfigParserBuilder.js'
import { FileNamed, realReadFilePair, realRequiredFilePair } from '../lib/FileNamed.js'
-import TestCustomService from '../__fixtures__/test-custom-service.js'
const log = logger('')
const __dirname = url.fileURLToPath(new URL('.', import.meta.url))
@@ -41,6 +40,8 @@ vi.mock('@babel/register', () => ({
default: vi.fn()
}))
+class CustomServiceOrReporter {}
+
/**
* Entirely in memory config structure to avoid reading the file system at all
*
@@ -70,7 +71,9 @@ const TestWdioConfig_AllInMemory = (): MockFileContent => ({
path.join(root, '/tests/validateConfig.test.ts'),
path.join(root, '/tests/..', 'src/index.ts')
]
- }
+ },
+ services: ['appium', [CustomServiceOrReporter, { foo: 'bar' }]],
+ reporters: ['spec', [CustomServiceOrReporter, { foo: 'bar' }]],
}
})
@@ -691,33 +694,48 @@ describe('ConfigParser', () => {
expect(configParser.getCapabilities()).toMatchObject([{ browserName: 'safari' }])
})
- it('should allow to specifying services', async () => {
+ it('should ensure there we do not duplicate class entries', async () => {
const configParser = await ConfigParserForTest(FIXTURES_CONF)
await configParser.initialize({
services: [
- [TestCustomService, { timestamp: 1709633731716 }]
- ]
- })
-
- const services = (await configParser.getConfig())['services']
- expect(services).toHaveLength(1)
- expect(services[0][1].timestamp).toBe(1709633731716)
- expect(typeof services[0][0]).toBe(typeof TestCustomService)
- })
-
- it('should allow to specifying reporters', async () => {
- const configParser = await ConfigParserForTest(FIXTURES_CONF)
- await configParser.initialize({
+ 'appium',
+ 'sauce',
+ [CustomServiceOrReporter, { foo: 'bar' }]
+ ],
reporters: [
- ['dot', 'spec']
+ 'dot',
+ 'spec',
+ [CustomServiceOrReporter, { foo: 'bar' }] as any
]
})
- const reporters = (await configParser.getConfig())['reporters']
- expect(reporters).toHaveLength(1)
- expect(reporters[0]).toHaveLength(2)
- expect(reporters[0][0]).toBe('dot')
- expect(reporters[0][1]).toBe('spec')
+ const { services, reporters } = configParser.getConfig()
+ expect(services).toHaveLength(3)
+ expect(services).toMatchInlineSnapshot(`
+ Set {
+ "appium",
+ "sauce",
+ [
+ [Function],
+ {
+ "foo": "bar",
+ },
+ ],
+ }
+ `)
+ expect(reporters).toHaveLength(3)
+ expect(reporters).toMatchInlineSnapshot(`
+ Set {
+ "dot",
+ "spec",
+ [
+ [Function],
+ {
+ "foo": "bar",
+ },
+ ],
+ }
+ `)
})
})What do you think?
Contributor
Author
|
@christian-bromann Thanks 🙏 changes seems looking good to me I'll implement this and update this PR soon. |
…gopani/webdriverio into fix/duplicate-service-run
Member
|
@jemishgopani let me know if there is anything I can do to help. |
Contributor
Author
|
@christian-bromann I have made some changes to your suggested code and updated the code. Whenever you have time please review this PR. |
Member
|
@jemishgopani mind raising the same PR for the |
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
12428
Types of changes
Checklist
Backport Request
Further comments
Reviewers: @webdriverio/project-committers