Skyux 'pact' CLI command #319

blackbaud-joshlandi · 2017-10-31T15:10:24Z

There's a lot of content here. First, what the command does is read a config setting called pactConfig for any entries into the pactConfig.pact array. It will spin up a pact server for each of these and then will spin up one more for the proxy server. The proxy server is meant to take requests from the browser which runs in localhost:9876 and changes the CORS headers so that they match what they would be if pacts were running in e2e's. The intended use is ${skyAppConfig.skyux.pactConfig.pactProxyServer}/provider; where provider is the string value of the provider that mocks your api. All ports are found using portfinder to prevent the known issue of the build agent failing when a port is already in use for another build. In a test with the extension pact-spec.ts the intended setup is

    TestBed.configureTestingModule({
      imports: [SkyAppTestModule]
    });
    skyPactService = TestBed.get(SkyPactService);

and for each service you call you will pass in the provider name when adding and removing interactions

skyPactService.addInteraction('email-ui-service', interaction);

Tests have not been written as I want to wait until the implementation is approved.

There is also a --watch option in case a watch is desired.

…nfig and tokenProvider to be overriden

…ing apps, and a util to help with passing information to user in AppConfig

…for pact settings in config file

codecov-io · 2017-10-31T15:10:28Z

Codecov Report

Merging #319 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #319    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          62     65     +3     
  Lines        1514   1631   +117     
  Branches      236    253    +17     
======================================
+ Hits         1514   1631   +117

Flag	Coverage Δ
#builder	`100% <100%> (ø)`	⬆️
#runtime	`100% <100%> (ø)`	⬆️
#srcapp	`100% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
config/karma/shared.karma.conf.js	`100% <ø> (ø)`	⬆️
config/webpack/test.webpack.config.js	`100% <ø> (ø)`	⬆️
lib/sky-pages-module-generator.js	`100% <100%> (ø)`	⬆️
runtime/config.ts	`100% <100%> (ø)`	⬆️
utils/pact-servers.js	`100% <100%> (ø)`
cli/pact.js	`100% <100%> (ø)`
index.js	`100% <100%> (ø)`	⬆️
config/karma/pact.karma.conf.js	`100% <100%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fcd826...cc87b63. Read the comment docs.

Blackbaud-SteveBrush · 2017-11-01T15:06:19Z

@blackbaud-joshlandi Looks like the coverage is off...

blackbaud-joshlandi · 2017-11-01T15:11:07Z

Yeah I know. That's because I haven't written tests. I'm waiting to see what reviewers have to say about the implementation details before I do that, otherwise I might be writing, re-writing, and re-re-writing tests.

Blackbaud-BobbyEarl

I think this direction makes sense. Aside from the initial feedback I've left, only major concern I have is how skyPagesConfig.skyux is being used. We may need to talk through that one.

I know you wanted feedback before writing tests. In the interim, I would suggest wiring up a test SPA via Engineering System and just have its package.json point to your branch blackbaud-joshlandi/skyux-builder#jlandi-pact-cli.

Blackbaud-BobbyEarl · 2017-11-03T14:27:10Z

cli/pact.js

+ * @name pact
+ */
+function pact(command, argv) {
+  const logger = require('../utils/logger');


I would suggest organizing this variable declaration section better. Typically I have my consts that require external modules, then internal, then lets

Blackbaud-BobbyEarl · 2017-11-03T14:29:05Z

cli/pact.js

+
+  let pactPortPromises = [];
+  // get a free port for every config entry, plus one for the proxy
+  for (let i = 0; i < skyPagesConfig.skyux.pactConfig.pacts.length + 1; i++) {


You'll need to make sure that skyPagesConfig.skyux.pactConfig.pacts exists before trying to use it.

Blackbaud-BobbyEarl · 2017-11-03T14:41:24Z

runtime/config.ts

@@ -40,8 +40,14 @@ export interface SkyuxConfigHost {
  url?: string;
 }

+export class SkyuxPactConfig {
+  public pacts?: any[];
+  public providers?: { [provider: string]: { host?: string; port?: string; fullUrl?: string; } };


Format this using multiple lines.

Blackbaud-BobbyEarl · 2017-11-03T14:45:53Z

utils/spec-bundle.js

@@ -62,7 +63,9 @@ Object.assign(global, testing);
 * any file that ends with spec.js and get its path. By passing in true
 * we say do this recursively
 */
-var testContext = require.context(ROOT_DIR, true, /\.spec\.ts/);
+var testContext = skyPagesConfig.runtime.command === 'pact' ?


I think only your regular expression needs to be changed depending on the command. No need to duplicate require.context.

So I tried this again, I also tried setting to regex to a variable, both gave this warning on a test run

WARNING in ../skyux-builder/utils/spec-bundle.js 66:18-25 Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

and tests failed.

Blackbaud-BobbyEarl · 2017-11-03T14:48:37Z

lib/sky-pages-module-generator.js

@@ -67,6 +76,11 @@ function getSource(skyAppConfig) {
    'SkyAppViewportService'
  ];

+  if (skyAppConfig.runtime.command === 'pact') {
+    runtimeProviders
+      .push('{ provide: SkyPactService, useClass: SkyPactService, deps: [SkyAppConfig] }');


Format this using multiple lines.

Blackbaud-BobbyEarl · 2017-11-03T14:50:02Z

lib/sky-pages-module-generator.js

  let runtimeProviders = [
    'SkyAppWindowRef',
    'SkyAppStyleLoader',
-    'SkyAuthTokenProvider',
+    skyAppConfig.runtime.command === 'pact' ?


Make this a variable and add to your if statement on line 51.

Blackbaud-BobbyEarl · 2017-11-03T14:50:26Z

lib/sky-pages-module-generator.js

@@ -67,6 +76,11 @@ function getSource(skyAppConfig) {
    'SkyAppViewportService'
  ];

+  if (skyAppConfig.runtime.command === 'pact') {
+    runtimeProviders


See if you can combine this with the if statement online 51, so you only have to compare the command once.

Blackbaud-BobbyEarl · 2017-11-03T14:53:20Z

config/karma/shared.karma.conf.js

  const path = require('path');
  let testWebpackConfig = require('../webpack/test.webpack.config');
  let remapIstanbul = require('remap-istanbul');
-  let skyPagesConfig = require('../sky-pages/sky-pages.config').getSkyPagesConfig(argv.command);
+  let skyPagesConfig = require('../sky-pages/sky-pages.config').getSkyPagesConfig(argv._[0]);


Can you explain this change? Looking at the code, this seems like a bug in builder as argv.command would never exist since the config file is having to read form the command line again. Can you elaborate on it?

Throwing a console.log right before that line gives { _: [ 'pact' ] }, and I checked it for existing commands like skyux test and it gives the same output. I noticed that and I changed it, I can test on a fresh copy of builder to be sure it's not related to any of my changes.

Alright, on a clean version it's still { _: [ 'test' ] }. I believe this is because argv is re-initialized in shared.karma.conf.js after argv.command is set in the cli command file.

Well good catch. This is a bug in builder that I'll plan on fixing outside the context of this PR.

Blackbaud-BobbyEarl · 2017-11-03T14:54:44Z

config/karma/pact.karma.conf.js

+  const path = require('path');
+  const pactServers = require('../../utils/pact-servers');
+
+  skyPagesConfig.skyux.pactConfig.providers = pactServers.getAllPactServers();


skyPagesConfig.skyux was designed to never be set from code - only set using the skyuxconfig.json files. For things set in code, I would move them to skyPagesConfig.runtime.X.

Blackbaud-BobbyEarl · 2017-11-03T15:04:16Z

Also, thank you very much for taking on this task. It's no easy feat and we certainly value all the hard-work and thought you've put into this functionality!

blackbaud-joshlandi · 2017-11-03T19:11:47Z

@Blackbaud-BobbyEarl I responded to all your comments, moved some logic to runtime as suggested. I think two comments probably warrant further talk, they're the only comments I wrote actual words in. I'll start spinning up a test SPA for this today, probably won't have it until Monday.

And of course, i'm glad to do it. It benefits me greatly, not to mention I'll become immortalized in the annals of SKYUX history when I get my Lego guy

…sure the latest is updated.

… pact as the DSL and pact-web

blackbaud-joshlandi · 2017-11-06T20:17:58Z

@Blackbaud-BobbyEarl SPA code is at
here

If you want to test it, clone: https://blackbaud.visualstudio.com/Products/_git/skyux-spa-test-jasmine-pacts

…s during testing.

…ting

…ndi/skyux-builder into jlandi-pact-cli

blackbaud-joshlandi · 2017-11-16T20:02:04Z

@Blackbaud-SteveBrush @Blackbaud-BobbyEarl coverage at 100% now. Take a look as soon as you can.

Blackbaud-BobbyEarl · 2017-11-21T13:47:56Z

Thanks @blackbaud-joshlandi, I'll review today. Do you mind resolving the conflict of lib/sky-pages-module-generator.js?

blackbaud-joshlandi · 2017-11-29T20:43:07Z

@Blackbaud-BobbyEarl I usually get email updates when people comment, sorry I didn't see your request to resolve conflicts 8 days ago. Just took care of it.

Blackbaud-BobbyEarl · 2017-11-30T14:55:42Z

Looks like something's not quite right after the merge @blackbaud-joshlandi.

blackbaud-joshlandi · 2017-11-30T15:27:50Z

@Blackbaud-BobbyEarl yeah, weird issue in that test where it expects pact to equal custom-command. Looking into it.

blackbaud-joshlandi · 2017-12-01T18:47:33Z

@Blackbaud-BobbyEarl resolved the issue. Poorly written afterEach in a test. Reminder you can clone https://blackbaud.visualstudio.com/Products/_git/skyux-spa-test-jasmine-pacts to test it out.

Blackbaud-BobbyEarl · 2017-12-04T14:02:38Z

lib/sky-pages-module-generator.js

+
+  runtimeProviders.push(authTokenProvider);
+
+  if (skyAppConfig.skyux.auth) {


Looks like maybe the merge from master didn't go quite right. This block is now duplicated around line 119.

I'm not entirely sure yet why this PR didn't fail, but I can duplicate your test SPA's failure locally.

Blackbaud-BobbyEarl · 2017-12-04T14:03:25Z

package.json

@@ -77,6 +77,9 @@
    "ngc-webpack": "3.0.0",
    "node-sass": "4.5.3",
    "open": "0.0.5",
+    "@pact-foundation/karma-pact": "^2.1.1",


For consistency with our other dependencies, let's use exact versions here (no caret).

…rats from package.json

blackbaud-joshlandi · 2017-12-05T20:32:36Z

@Blackbaud-BobbyEarl i believe i took care of the final two comments. let me know if you have any other concerns

Blackbaud-BobbyEarl

Thanks @blackbaud-joshlandi! I'm going to approve and merge this in shortly. Let's work with @blackbaud-johnly to get some documentation.

blackbaud-johnly · 2017-12-11T14:22:35Z

I just created blackbaud/skyux2#1354 for the docs.

* trying to bypass some stuff * overriding some methods for testing * workring on changes to configs and cli commands. Still need to get config and tokenProvider to be overriden * dynamically adding pact ports * adding more karma config settings, adding a service for use my consuming apps, and a util to help with passing information to user in AppConfig * Adding documentation, watch option for command, and making interface for pact settings in config file * supporting looking for only pact-spec files * respecting a bunch of jslint errors when running npm run test * optionally let use define directory for pact json and pact logs * changing import statement * formatting changes and moving dynamically set config settings to runtime * karma-pact depends on the latest pact-node, but our config doesn't ensure the latest is updated. * letting karma-pact take care of it's pact-node dependency. Still need pact as the DSL and pact-web * cleaning up some formatting so build passes * noticed ports weren't being retreived across processes. portfinder doesn't look at host 0:0:0:0 and also promises need to be resolved sequentially to prevent promises resolving on same port since they don't get consumed until after portfinder starts. * adding spec file for pacts * writing tests for pact command, made some changes based on my findings during testing. * tests on karma config, and further changes for thing found during testing * for whatever reason, not including this causes errors. * 100% coverage * some linting errors getting fixed * removing that one extra line * thinking not stopping all mocks here could've caused the issue * weird merge conflict that didn't cause build to fail, and removing carats from package.json * possibly no need to have the pact-web package * right, this test needs to be updated

Josh Landi and others added 13 commits October 10, 2017 15:55

trying to bypass some stuff

f603c82

overriding some methods for testing

a033aea

merging with master

fd6b4d1

workring on changes to configs and cli commands. Still need to get co…

8d2d768

…nfig and tokenProvider to be overriden

dynamically adding pact ports

6da0cfb

Merge branch 'master' of https://github.com/blackbaud/skyux-builder

de07822

adding more karma config settings, adding a service for use my consum…

16d4b26

…ing apps, and a util to help with passing information to user in AppConfig

Merge branch 'master' of https://github.com/blackbaud/skyux-builder

786ab3d

Adding documentation, watch option for command, and making interface …

224bd06

…for pact settings in config file

supporting looking for only pact-spec files

1abd6bc

respecting a bunch of jslint errors when running npm run test

004b726

optionally let use define directory for pact json and pact logs

f40192f

Merge branch 'master' into master

fd0d91d

Josh Landi and others added 2 commits October 31, 2017 11:13

changing import statement

dbe6d27

Merge branch 'master' into jlandi-pact-cli

64eeacb

blackbaud-joshlandi added 2 commits November 2, 2017 08:25

Merge branch 'master' into jlandi-pact-cli

f34ff9d

Merge branch 'master' into jlandi-pact-cli

fcd96e5

Blackbaud-BobbyEarl reviewed Nov 3, 2017

View reviewed changes

Josh Landi and others added 3 commits November 3, 2017 15:06

formatting changes and moving dynamically set config settings to runtime

c9c8a11

Merge branch 'master' into jlandi-pact-cli

e4460aa

Merge branch 'master' into jlandi-pact-cli

1571f46

Josh Landi added 3 commits November 6, 2017 11:30

karma-pact depends on the latest pact-node, but our config doesn't en…

63edd3b

…sure the latest is updated.

letting karma-pact take care of it's pact-node dependency. Still need…

fb44a40

… pact as the DSL and pact-web

cleaning up some formatting so build passes

ba4eee4

Josh Landi and others added 8 commits November 14, 2017 11:36

adding spec file for pacts

89a03af

writing tests for pact command, made some changes based on my finding…

0ef0555

…s during testing.

Merge branch 'master' into jlandi-pact-cli

0fb6bfd

tests on karma config, and further changes for thing found during tes…

fbac696

…ting

Merge branch 'jlandi-pact-cli' of https://github.com/blackbaud-joshla…

7f9256e

…ndi/skyux-builder into jlandi-pact-cli

for whatever reason, not including this causes errors.

209927c

100% coverage

048a877

some linting errors getting fixed

009ee52

blackbaud-joshlandi added 2 commits November 29, 2017 15:41

Merge branch 'master' into jlandi-pact-cli

7183507

Merge branch 'master' into jlandi-pact-cli

d83fa73

removing that one extra line

faab3f7

thinking not stopping all mocks here could've caused the issue

32de8f8

Blackbaud-BobbyEarl reviewed Dec 4, 2017

View reviewed changes

weird merge conflict that didn't cause build to fail, and removing ca…

1892622

…rats from package.json

Josh Landi and others added 3 commits December 6, 2017 10:17

possibly no need to have the pact-web package

8fc35c5

right, this test needs to be updated

424e6fe

Merge branch 'master' into jlandi-pact-cli

cc87b63

Blackbaud-BobbyEarl approved these changes Dec 11, 2017

View reviewed changes

Blackbaud-BobbyEarl merged commit b5eb1a2 into blackbaud:master Dec 11, 2017

blackbaud-johnly mentioned this pull request Dec 11, 2017

Document pact CLI command blackbaud/skyux2#1354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skyux 'pact' CLI command #319

Skyux 'pact' CLI command #319

blackbaud-joshlandi commented Oct 31, 2017

codecov-io commented Oct 31, 2017 •

edited

Loading

Blackbaud-SteveBrush commented Nov 1, 2017

blackbaud-joshlandi commented Nov 1, 2017

Blackbaud-BobbyEarl left a comment

Blackbaud-BobbyEarl Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

Blackbaud-BobbyEarl Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

Blackbaud-BobbyEarl Nov 15, 2017

Blackbaud-BobbyEarl Nov 3, 2017

blackbaud-joshlandi Nov 3, 2017

Blackbaud-BobbyEarl commented Nov 3, 2017

blackbaud-joshlandi commented Nov 3, 2017 •

edited

Loading

blackbaud-joshlandi commented Nov 6, 2017 •

edited

Loading

blackbaud-joshlandi commented Nov 16, 2017

Blackbaud-BobbyEarl commented Nov 21, 2017

blackbaud-joshlandi commented Nov 29, 2017

Blackbaud-BobbyEarl commented Nov 30, 2017

blackbaud-joshlandi commented Nov 30, 2017

blackbaud-joshlandi commented Dec 1, 2017

Blackbaud-BobbyEarl Dec 4, 2017

Blackbaud-BobbyEarl Dec 4, 2017

blackbaud-joshlandi commented Dec 5, 2017

Blackbaud-BobbyEarl left a comment

blackbaud-johnly commented Dec 11, 2017


		runtimeProviders.push(authTokenProvider);

		if (skyAppConfig.skyux.auth) {

Skyux 'pact' CLI command #319

Skyux 'pact' CLI command #319

Conversation

blackbaud-joshlandi commented Oct 31, 2017

codecov-io commented Oct 31, 2017 • edited Loading

Codecov Report

Blackbaud-SteveBrush commented Nov 1, 2017

blackbaud-joshlandi commented Nov 1, 2017

Blackbaud-BobbyEarl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blackbaud-BobbyEarl commented Nov 3, 2017

blackbaud-joshlandi commented Nov 3, 2017 • edited Loading

blackbaud-joshlandi commented Nov 6, 2017 • edited Loading

blackbaud-joshlandi commented Nov 16, 2017

Blackbaud-BobbyEarl commented Nov 21, 2017

blackbaud-joshlandi commented Nov 29, 2017

Blackbaud-BobbyEarl commented Nov 30, 2017

blackbaud-joshlandi commented Nov 30, 2017

blackbaud-joshlandi commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackbaud-joshlandi commented Dec 5, 2017

Blackbaud-BobbyEarl left a comment

Choose a reason for hiding this comment

blackbaud-johnly commented Dec 11, 2017

codecov-io commented Oct 31, 2017 •

edited

Loading

blackbaud-joshlandi commented Nov 3, 2017 •

edited

Loading

blackbaud-joshlandi commented Nov 6, 2017 •

edited

Loading