Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
robmoffat
changed the title
|
|
|
|
Member
Author
|
@kriswest the other day you asked if we could make sure the names of the tests line up with the documentation. I've done some analysis and it seems like they already are.
Am I missing something? |
Contributor
|
They weren't matched at one point - I guess we had them change them! I think in an earlier version they created text descriptions of each test and used those. I will endeavour to remember that this was sorted out already 😉
…--
[cid:3bdcf451-8f39-49c7-9ea8-74170e53ac68]
Kris West (he/him)
Principal Engineer | Open Source Program Office (OSPO)
***@***.******@***.***> | +44 7715104292
250 Bishopsgate, London, EC2M 4AA
natwestgroup.com
The information classification of this email is Confidential unless otherwise stated.
[cid:854a3dd4-9f86-4b53-b02c-6ffd435dbbde]
Do you have a question?
Join us on Discourse https://natwest.discourse.group<https://natwest.discourse.group/> (Open Source<https://natwest.discourse.group/c/open-source/57> | InnerSource<https://natwest.discourse.group/c/inner-sourcing/58>)
________________________________
From: Rob Moffat ***@***.***>
Sent: 25 April 2025 10:08 AM
To: finos/FDC3 ***@***.***>
Cc: West, Kristopher (Enterprise Engineering, Technology) ***@***.***>; Mention ***@***.***>
Subject: Re: [finos/FDC3] Port of FDC3 Conformance to Monorepo (PR #1576)
*********************************************
"This is an external email. Do you know who has sent it? Can you be sure that any links and attachments contained within it are safe? If in any doubt, use the Phishing Reporter Button in your Outlook client or forward the email to ~ I've Been Phished"
*********************************************
[https://avatars.githubusercontent.com/u/568673?s=20&v=4]robmoffat left a comment (finos/FDC3#1576)<#1576 (comment)>
@kriswest<https://github.com/kriswest> the other day you asked if we could make sure the names of the tests line up with the documentation. I've done some analysis and it seems like they already are.
Test ID TypeScript Source File Present Name if Different
GetAgentAPI Not found No -
BasicCL1 fdc3.basic.ts Yes -
BasicCL2 fdc3.basic.ts Yes -
BasicIL1 fdc3.basic.ts Yes -
BasicGI1 fdc3.basic.ts Yes -
BasicAC1 fdc3.basic.ts Yes -
BasicUC1 fdc3.basic.ts Yes -
BasicJC1 fdc3.basic.ts Yes -
BasicRI1 fdc3.basic.ts Yes -
BasicRI2 fdc3.basic.ts Yes -
2.0-ACBasicUsage1 fdc3.app-channels.ts Yes -
2.0-ACBasicUsage2 fdc3.app-channels.ts Yes -
2.0-ACFilteredContext1 fdc3.app-channels.ts Yes -
2.0-ACFilteredContext2 fdc3.app-channels.ts Yes -
2.0-ACFilteredContext3 fdc3.app-channels.ts Yes -
2.0-ACFilteredContext4 fdc3.app-channels.ts Yes -
2.0-ACUnsubscribe fdc3.app-channels.ts Yes -
2.0-ACContextHistoryTyped fdc3.app-channels.ts Yes -
2.0-ACContextHistoryMultiple fdc3.app-channels.ts Yes -
2.0-ACContextHistoryLast fdc3.app-channels.ts Yes -
2.0-UCBasicUsage1 fdc3.user-channels.ts Yes -
2.0-UCBasicUsage2 fdc3.user-channels.ts Yes -
2.0-UCBasicUsage3 fdc3.user-channels.ts Yes -
2.0-UCBasicUsage4 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage1 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage2 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage3 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage4 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage5 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsage6 fdc3.user-channels.ts Yes -
2.0-UCFilteredUsageChange fdc3.user-channels.ts Yes -
2.0-UCFilteredUsageUnsubscribe fdc3.user-channels.ts Yes -
2.0-UCFilteredUsageLeave fdc3.user-channels.ts Yes -
2.0-UCFilteredUsageNoJoin fdc3.user-channels.ts Yes -
2.2-ChannelChangedEvent Not found No -
2.0-GetAppMetadata fdc3.getAppMetadata.ts Yes -
2.0-AppInstanceMetadata fdc3.getAppMetadata.ts Yes -
2.0-FindInstances fdc3.findInstances.ts Yes -
2.0-GetInfo1 fdc3.getInfo.ts Yes -
2.0-GetInfo2 fdc3.getInfo.ts Yes -
2.0-AOpensB3 fdc3.open.ts Yes -
2.0-AOpensB4 fdc3.open.ts Yes -
2.0-AFailsToOpenB3 fdc3.open.ts Yes -
2.0-AOpensBWithContext3 fdc3.open.ts Yes -
2.0-AOpensBWithSpecificContext fdc3.open.ts Yes -
2.0-AOpensBMultipleListen fdc3.open.ts Yes -
2.0-AOpensBWithWrongContext fdc3.open.ts Yes -
2.0-FindIntentAppD fdc3.findIntent.ts Yes -
2.0-FindNonExistentIntentAppD fdc3.findIntent.ts Yes -
2.0-FindIntentAppDRightContext fdc3.findIntent.ts Yes -
2.0-FindIntentAppDWrongContext fdc3.findIntent.ts Yes -
2.0-FindIntentAppDMultiple1 fdc3.findIntent.ts Yes -
2.0-FindIntentAppDMultiple2 fdc3.findIntent.ts Yes -
2.0-FindIntentByContextSingleContext fdc3.findIntentsByContext.ts Yes -
2.0-FindIntentByContextWrongIntentAppD fdc3.findIntentsByContext.ts Yes -
2.0-FindIntentAppDByResultSingle fdc3.findIntent.ts Yes -
2.0-FindIntentAppDByResultSingleNullContext fdc3.findIntent.ts Yes -
2.0-FindIntentAppDByResultMultiple fdc3.findIntent.ts Yes -
2.0-FindIntentAppDByResultChannel1 fdc3.findIntent.ts Yes -
2.0-FindIntentAppDByResultChannel2 fdc3.findIntent.ts Yes -
2.0-RaiseIntentSingleResolve fdc3.raiseIntent.ts Yes -
2.0-RaiseIntentTargetedAppResolve fdc3.raiseIntent.ts Yes -
2.0-RaiseIntentTargetedInstanceResolveOpen fdc3.raiseIntent.ts Yes -
2.0-RaiseIntentTargetedInstanceResolveFindInstances fdc3.raiseIntent.ts Yes -
2.0-RaiseIntentFailedResolve fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppResolve1 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppResolve2 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppResolve3 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppResolve4 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppInstanceResolve1 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentFailTargetedAppInstanceResolve2 fdc3.raiseIntent-NoAppsFound.ts Yes -
2.0-RaiseIntentVoidResult0secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentVoidResult5secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentVoidResult61secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentContextResult0secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentContextResult5secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentContextResult61secs fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentChannelResult fdc3.raiseIntent-Result.ts Yes -
2.0-RaiseIntentPrivateChannelResult fdc3.raiseIntent-Result.ts Yes -
2.0-PrivateChannelsAreNotAppChannels fdc3.raiseIntent.ts Yes -
2.0-PrivateChannelsLifecycleEvents fdc3.raiseIntent.ts Yes -
2.0-ResolveAmbiguousIntentTarget fdc3.manual.ts Yes -
2.0-ResolveAmbiguousContextTarget fdc3.manual.ts Yes -
2.0-ResolveAmbiguousIntentTargetMultiInstance fdc3.manual.ts Yes -
2.0-ResolveAmbiguousContextTargetMultiInstance fdc3.manual.ts Yes -
Am I missing something?
—
Reply to this email directly, view it on GitHub<#1576 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAM7PBEHSAECYG4OYJQDZND23H3SNAVCNFSM6AAAAAB3HQSU4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMRZHAZDCOBYHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
National Westminster Bank plc. Registered in England No. 929027. Registered Office: 250 Bishopsgate, London EC2M 4AA. National Westminster Bank plc is authorised by the Prudential Regulation Authority, and regulated by the Financial Conduct Authority and the Prudential Regulation Authority.
The Royal Bank of Scotland plc. Registered in Scotland No. 83026. Registered Office: 36 St Andrew Square, Edinburgh EH2 2YB. The Royal Bank of Scotland plc is authorised by the Prudential Regulation Authority, and regulated by the Financial Conduct Authority and the Prudential Regulation Authority.
The Royal Bank of Scotland plc and National Westminster Bank plc are authorised to act as agent for each other.
The Royal Bank of Scotland plc and National Westminster Bank plc are UK chartered banks and are not chartered or licensed as banks by the United States or any individual state.
This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the message from your computer. Internet e-mails are not necessarily secure. The Royal Bank of Scotland plc, National Westminster Bank plc or any affiliated entity (“NatWest” or “us”) does not accept responsibility for changes made to this message after it was sent. NatWest may monitor e-mails for business and operational purposes. By replying to this message you give your consent to the monitoring of your e-mail communications with us. Whilst all reasonable care has been taken to avoid the transmission of viruses, it is the responsibility of the recipient to ensure that the onward transmission, opening or use of this message and any attachments will not adversely affect its systems or data. No responsibility is accepted by NatWest in this regard and the recipient should carry out such virus and other checks as it considers appropriate.
|
Contributor
julianna-ciq
left a comment
julianna-ciq
left a comment
There was a problem hiding this comment.
review of everything but the markdown files
I added suggestions for some typescript stuff - I tried to reduce the amount of as I saw. fyi @robmoffat @kriswest
| NoListenerTimeout: 120000, // the amount of time to allow for a DA to timeout waiting on a context or intent listener | ||
| // FDC3 does not define this timeout so this should be extended if the DA uses a longer timeout | ||
| ControlChannel: 'app-control', //app channel used for passing messages between mock apps and tests | ||
| } as const; |
Contributor
There was a problem hiding this comment.
What is the as const for?
| @@ -0,0 +1,26 @@ | |||
| /** same in 1.2 and 2.0 */ | |||
| export interface CommonContext { | |||
Contributor
There was a problem hiding this comment.
Would it make sense to use existing types from the monorepo? In the generated BrowserTypes file, I think the interface Context is the same as this one.
Member
Author
There was a problem hiding this comment.
remember this is just porting existing code to our monorepo. If we wanted to start fixing things, well.... that could be a long PR
| import { IChannelService } from './interfaces'; | ||
|
|
||
| export class Fdc3CommandExecutor { | ||
| async executeCommands(orderedCommands: string[], config: ChannelsAppConfig, channelService: IChannelService<any>) { |
Contributor
There was a problem hiding this comment.
Anti-pattern to use any. Is there something better we can use? Maybe a list of possible channel types? If not, can we change to unknown?
Member
Author
There was a problem hiding this comment.
see my above comment - this is a minimal PR to get the repo included.
| } | ||
| case commands.broadcastInstrumentContext: { | ||
| const contextType = config.contextId ? `fdc3.instrument.${config.contextId}` : 'fdc3.instrument'; | ||
| await channelService.broadcastContextItem(contextType, channel, config.historyItems ?? 1, config.testId); |
Contributor
There was a problem hiding this comment.
It's possible for channel to be undefined. Will that cause problems?
Member
Author
There was a problem hiding this comment.
you can give it a try!
| } | ||
| case commands.broadcastContactContext: { | ||
| const contextType = config.contextId ? `fdc3.contact.${config.contextId}` : 'fdc3.contact'; | ||
| await channelService.broadcastContextItem(contextType, channel, config.historyItems ?? 1, config.testId); |
Contributor
There was a problem hiding this comment.
It's possible for channel to be undefined. Will that cause problems?
Comment on lines
+18
to
+21
| appControlChannel.broadcast({ | ||
| type: 'closeWindow', | ||
| testId: currentTest, | ||
| } as AppControlContext); |
Contributor
There was a problem hiding this comment.
Suggested change
| appControlChannel.broadcast({ | |
| type: 'closeWindow', | |
| testId: currentTest, | |
| } as AppControlContext); | |
| const testContext: AppControlContext = { | |
| type: 'closeWindow', | |
| testId: currentTest, | |
| }; | |
| appControlChannel.broadcast(testContext); |
Comment on lines
+14
to
+17
| await sendContextToTests(fdc3, { | ||
| type: ControlContextType.CONTEXT_RECEIVED, | ||
| context: context, | ||
| } as AppControlContext); |
Contributor
There was a problem hiding this comment.
Suggested change
| await sendContextToTests(fdc3, { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| context: context, | |
| } as AppControlContext); | |
| const testContext: AppControlContext = { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| context: context, | |
| }; | |
| await sendContextToTests(fdc3, testContext); |
Comment on lines
+22
to
+25
| await sendContextToTests(fdc3, { | ||
| type: ControlContextType.CONTEXT_RECEIVED, | ||
| errorMessage: message, | ||
| } as AppControlContext); |
Contributor
There was a problem hiding this comment.
Suggested change
| await sendContextToTests(fdc3, { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| errorMessage: message, | |
| } as AppControlContext); | |
| const testContext: AppControlContext = { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| errorMessage: message, | |
| }; | |
| await sendContextToTests(fdc3, testContext); |
Comment on lines
+9
to
+12
| await appControlChannel.broadcast({ | ||
| type: 'windowClosed', | ||
| testId: context.testId, | ||
| } as AppControlContext); |
Contributor
There was a problem hiding this comment.
Suggested change
| await appControlChannel.broadcast({ | |
| type: 'windowClosed', | |
| testId: context.testId, | |
| } as AppControlContext); | |
| const testContext: AppControlContext = { | |
| type: 'windowClosed', | |
| testId: context.testId, | |
| }; | |
| await appControlChannel.broadcast(testContext); |
Comment on lines
+12
to
+15
| await sendContextToTests(fdc3, { | ||
| type: ControlContextType.CONTEXT_RECEIVED, | ||
| errorMessage: `Listener received incorrect context type. Listener listening for 'shouldNotReceiveThisContext' type received '${context.type}' type`, | ||
| } as AppControlContext); |
Contributor
There was a problem hiding this comment.
Suggested change
| await sendContextToTests(fdc3, { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| errorMessage: `Listener received incorrect context type. Listener listening for 'shouldNotReceiveThisContext' type received '${context.type}' type`, | |
| } as AppControlContext); | |
| const testContext: AppControlContext = { | |
| type: ControlContextType.CONTEXT_RECEIVED, | |
| errorMessage: `Listener received incorrect context type. Listener listening for 'shouldNotReceiveThisContext' type received '${context.type}' type`, | |
| }; | |
| await sendContextToTests(fdc3, testContext); |
17 tasks
robmoffat
changed the title
Co-authored-by: Julianna Langston <74684272+julianna-ciq@users.noreply.github.com>
Co-authored-by: Julianna Langston <74684272+julianna-ciq@users.noreply.github.com>
Co-authored-by: Julianna Langston <74684272+julianna-ciq@users.noreply.github.com>
Contributor
|
My remaining comments would be classed as improvements. I believe that this PR is good to move forward, and I'll pick up the improvements in a separate PR. |
julianna-ciq
previously approved these changes
Jun 19, 2025
|
|
17 tasks
julianna-ciq
approved these changes
Aug 13, 2025
|
|
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.
Describe your change
getAgent.Review Checklist