Adding protractor configuration #57

el-davo · 2017-07-18T18:32:13Z

Following discussion on - #56 here is the pr. Let me know what you think?

gonzofish · 2017-07-19T14:47:46Z

I'm going to take a look at this tonight.

After an initial look, I think we might need to make the approach more general for scaffolding, but since I don't really know Protractor or its best practices, I may be wrong.

gonzofish · 2017-07-19T14:54:59Z

commands/initial/templates/package.json

-    "test": "node ./tasks/test"
+    "test": "node ./tasks/test",
+    "pree2e": "webdriver-manager update",
+    "e2e": "protractor"


Can protractor be run via a JS API? That way devs could run npm test e2e or npm run e2e...using npm test just keeps all of the testing in a single paradigm, which some might find nice

If i understand correctly you want to run the protractor tests along with the unit tests? I think that would cause a few issues

Protractor tests are very expensive to run. and would take a lot more time to run then unit tests.

When the user clicks save the tests most likely would not have finished executing before the next save

I think running protractor tests on save would be very annoying for the developer as it would be opening real browsers, basically robbing control from the user until the tests completed.

Keeping the tests in separate scripts is the recommended approach of the angular-cli

Oh, sorry, I didn't mean along with unit tests. I do understand that protractor testes can be time-consuming.

What I meant was would we be able to leverage the current tasks/test.js file to execute the e2e tests? That way we keep the tests running all in one file.

@gonzofish A seperated script for e2e tests is always better.
The reason lies within the fact that normally you would to do the following steps for a library (e.g. on a CI machine or during local development) and you wanna have a dedicated task for everyone of them:

Build: npm run build

Unit Test : npm run test

serve (local) resp. deploy (CI): npm run serve/deploy

e2e test: npm run e2e

The last step should have at least the possibility to specify the selenium server used ( e.g. localhost:4444 for local development and any other running selenium server when on CI).

So it's always better to have two scripts/tasks for the two different types of test, due to their needed tasks that should in between (serve resp. deploy)

@nvdnkpr thanks for your continued participation in this project!

wouldn't being able to run npm test e2e satisfy the same thing as npm run e2e? We can update the test script to understand the arguments passed to it/add a configuration file so that the Selenium server port can be customized.

Does that satisfy your concerns?

It's less about my concerns but more about simplicity. You can of course do what you describe but that somehow pulls all the test related parameters into your ./tasks/test script.

But NPM enables one to append configuration by appending it after a double dash after run command e.g. npm run e2e -- --seleniumServerAddress ADDRESS which gets resolved to protractor --seleniumServerAddress ADDRESS which then overwrites the default seleniumServerAddress (or the one from the configuration) with the new value ADDRESS.
(That's why most of the tools have the precedence in the following order: CLI > configuration > user-dotfile > default value)

This powermove of NPM enables you (the developer) to just run npm run e2e locally to run the tests (in this case: default) selenium server. In your CI environment however you then run npm run e2e -- --seleniumServerAddress ADDRESS to adjust your build script to the CI environment.

gonzofish

Again, this is super awesome! I hope I don't come off as picky and controlling!

Most of the comments are more discussion than changes...

gonzofish · 2017-07-19T15:07:22Z

commands/initial/templates/webpack/webpack.dev.js

@@ -29,7 +29,6 @@ module.exports = webpackCommon('dev', {
    devtool: 'cheap-module-eval-source-map',
    entry: {
        app: [ examplePath('example.main') ],
-        scripts: [],


We need to keep the scripts attribute in webpack.dev.js

Webpack reports the following for me when trying to run it with scripts as an empty array

WebpackOptionsValidationError: Invalid configuration object. Webpack has been initialised using a configuration object that does not match the API schema. - configuration.entry should be one of these: object { <key>: non-empty string | [non-empty string] } | non-empty string | [non-empty string] | function The entry point(s) of the compilation. Details: * configuration.entry['scripts'] should be a string. * configuration.entry['scripts'] should not be empty. * configuration.entry['scripts'] should be one of these: non-empty string | [non-empty string] * configuration.entry should be a string. * configuration.entry should be an array:

I'm not sure what to do about this one, but it looks like a misconfiguration?

Oh really? Interesting. Maybe that was an update. We'll keep it out for now...I might have even made that change, I can't remember 😉

gonzofish · 2017-07-19T15:08:18Z

commands/initial/templates/examples/example.component.html

@@ -0,0 +1 @@
+<div class="welcome">Welcome</div>


What does this HTML page do? Does it act as the index.html-type page where the tests are run?

The intent here was that we have a default example, much like the angular-cli. That was also the intent of the default test below. I can remove if not needed

Yeah, we can scaffold out the skeleton of it and let devs add their own code. I think of how the CLI scaffolds out your initial components & HTML...it annoys me that everytime I create an app I have to remove all those files.

gonzofish · 2017-07-19T15:13:23Z

commands/initial/templates/protractor.conf.js

@@ -0,0 +1,39 @@
+const {SpecReporter} = require('jasmine-spec-reporter');


Just like Webpack & Karma configs, this configuration should be extensible by devs using the library. So if they place a protractor.conf.js file in their configs directory, this config can be altered.

We'd need to identify the set of attributes the dev could override and which ones angular-librarian should be in charge of.

gonzofish · 2017-07-19T15:15:35Z

commands/initial/templates/e2e/example.e2e-spec.ts

+    beforeEach(() => examplePo.go());
+
+    it('should welcome user to the example page', () => {
+        expect(examplePo.welcome.getText()).toBe('Welcome');


I'm not generally a fan of default tests. I'd even like to get rid of the ones added to scaffolded unit tests but they're good sanity checks.

For this, it seems that anytime a library is generated, the dev has to remove this test--is that true? If it is, can we remove it and keep the rest of the setup?

I know what you mean but they come with an advantage and at a cost:
The cost is that what you want to get rid of, the example app/spec/whatever.
The advantage is that your setup is already ready to go, you just need to overwrite the one test case you wanted to get rid off.
But I actually understand that for some who is like generating the 30th component he doesn't want to delete the example stuff all over again. That's why I would suggest adding a flag to every of your generating-component/directive/service/pipe script that is boolean and is named "--with--example" (or --without--example).

If you use something like rc then you can even use a configuration file that behaves like I described above (CLI > configuration > ...)

gonzofish · 2017-07-19T15:16:30Z

commands/initial/templates/protractor.conf.js

+        './e2e/**/*.e2e-spec.ts'
+    ],
+    capabilities: {
+        'browserName': 'chrome'


I know I'm being picky, but these quotes aren't needed, right? If not, I like to keep attribute names unquoted unless necessary.

Whoops yes agree with you here. will remove

el-davo · 2017-07-19T19:23:48Z

Thanks for the awesome feedback. Again please feel free to decline this pr if you feel it can be improved. I will gladly give it another stab at a future date. I wish i had more time to commit to this 🥇

gonzofish · 2017-07-19T22:14:11Z

Thanks for contributing. It's really nice to have people commit time to something you've done. I think the changes are pretty light, so do them when you have a shot, no rush.

el-davo added 2 commits July 18, 2017 19:30

Adding protractor configuration

eecc717

Updating jasmine-spec-reporter to 4.1.1

2c4b24d

gonzofish reviewed Jul 19, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding protractor configuration #57

Adding protractor configuration #57

el-davo commented Jul 18, 2017

gonzofish commented Jul 19, 2017

gonzofish Jul 19, 2017

el-davo Jul 19, 2017

gonzofish Jul 19, 2017

nvdnkpr Aug 14, 2017

gonzofish Aug 15, 2017

nvdnkpr Aug 15, 2017

gonzofish left a comment

gonzofish Jul 19, 2017

el-davo Jul 19, 2017 •

edited

Loading

gonzofish Jul 19, 2017

gonzofish Jul 19, 2017

el-davo Jul 19, 2017 •

edited

Loading

gonzofish Jul 19, 2017

gonzofish Jul 19, 2017

gonzofish Jul 19, 2017

nvdnkpr Aug 15, 2017

gonzofish Jul 19, 2017

el-davo Jul 19, 2017

el-davo commented Jul 19, 2017

gonzofish commented Jul 19, 2017

		@@ -0,0 +1,39 @@
		const {SpecReporter} = require('jasmine-spec-reporter');

Adding protractor configuration #57

Are you sure you want to change the base?

Adding protractor configuration #57

Conversation

el-davo commented Jul 18, 2017

gonzofish commented Jul 19, 2017

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

gonzofish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

el-davo Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

el-davo Jul 19, 2017 • edited Loading

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

el-davo commented Jul 19, 2017

gonzofish commented Jul 19, 2017

el-davo Jul 19, 2017 •

edited

Loading

el-davo Jul 19, 2017 •

edited

Loading