Skip to content
This repository has been archived by the owner on Dec 17, 2018. It is now read-only.

Adding protractor configuration #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

el-davo
Copy link
Contributor

@el-davo el-davo commented Jul 18, 2017

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

@gonzofish
Copy link
Owner

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.

"test": "node ./tasks/test"
"test": "node ./tasks/test",
"pree2e": "webdriver-manager update",
"e2e": "protractor"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. Build: npm run build
  2. Unit Test : npm run test
  3. serve (local) resp. deploy (CI): npm run serve/deploy
  4. 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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@gonzofish gonzofish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@el-davo el-davo Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@el-davo el-davo Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

beforeEach(() => examplePo.go());

it('should welcome user to the example page', () => {
expect(examplePo.welcome.getText()).toBe('Welcome');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 > ...)

'./e2e/**/*.e2e-spec.ts'
],
capabilities: {
'browserName': 'chrome'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops yes agree with you here. will remove

@el-davo
Copy link
Contributor Author

el-davo commented Jul 19, 2017

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
Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants