Update testing commands #97

SirDarquan · 2018-02-04T22:10:29Z

fixes #96

…ride options

gonzofish · 2018-02-25T00:54:49Z

commands/initial/templates/tasks/test.js

+
+	args.forEach((arg, index)=>{
+		let value = arg.match(/^(\w+)/)[1];
+		switch(value){	//there are probably libraries that parse cmd line arguments...


there are, but when I started this, the intent was to keep it as simple & lightweight as possible, as you might see, there aren't a ton of external includes from Librarian code

I wanted to mention, Librarian has its own focused utility for handling options:
https://github.com/gonzofish/angular-librarian/blob/master/tools/utilities/options.js

Here's an example of how its used:
https://github.com/gonzofish/angular-librarian/blob/master/commands/component/index.js#L20

Again, totally understand there are libraries that do this, but when I started Librarian I was just having fun and rolled my own--the options.js file is fully tested, though:
https://github.com/gonzofish/angular-librarian/blob/master/test/tools/utilities/options.spec.js

Oh! I didn't realize you had code to handle that. The original code wasn't using it either and is the only reason I suggested finding a library. I'll see what I can do.

Ok now I've tried to incorporate options.js and my inexperience with writing developer tools may be biting me in the butt. This file (test.js) gets moved to the host library while options.js stays in the angular-librarian folder. This gives it two different requires paths: one to allow the library to build ('../../../../tools/utilities') and one from the host library ('angular-librarian/tools/utilities'). Is there something in the initialization that will resolve this automatically or am I supposed to pretend that test.js is in a host library and use that path?

This file, test.js, ends up as part of the library's code base, as an NPM script. So the proper thing to do would be to access it using angular-librarian/tools/utilities (or angular-librarian/tools/utilities/options)

Now I've attempted to use the options.js file in test.js and I have some reservations about it. I've updated the code to conform to the style guide lines you've given me. Is using the options code a deal breaker? Because I'd like to ask a few questions about that separately.

gonzofish

First, thank you so much for the PR and sorry I took so long to get back (I hope you're still interested in helping)

I hope I don't come off harsh here, that's never my aim. I really like the flexibility this is aiming to give the developers using Librarian.

The changes requested are basically:

Keep the default npm test tied to running headless tests once
Keep closer to the styling (note that I know that we're not enforcing it through code, a la ESLint or Prettier, but I'll take care of that after merging your PR)
Suggestions to try to leverage the options.js utility where possible

Again, thank you SO much for contributing!

gonzofish · 2018-02-25T00:55:15Z

README.md

-watch (alias: w)| Run through Chrome with files being watched & tests automatically re-run
-
-Note that Chrome still requires a manual refresh on the Debug tab to see updated test results.
+default | Run through defined browsers one time with no file watching


The default option needs to remain headless, as the publish command leverages npm test to run. Part of the reason for the defaults, also is to provide convention to developers over configuration. By providing the defaults, we keep users from having to extend karma if they don't want to.

I'm starting to see a conflict that I'm not sure how to reconcile. So I'll present that and we can decide how to proceed.

When I began trying to understand how client-side testing should should work, PhantomJS was there to help fill that gap needed for headless testing. But because it's not an actual browser, some implementations didn't work as expected in real browsers. Since then, browser makers have added facilities to allow developers to use actual browsers during tests which also gave rise to Selenium Grid providers like BrowserStack, SauceLabs and others. Most, if not all offer a free version of their service of some kind. This further pushes the need to use PhantomJS in the background.

This is where I was when I suggested these changes. I actually use SauceLabs for my project and don't want to use PhantomJS at all on my CI server in case something PhantomJS doesn't support well fails. (In my case PhantomJS is fine but I was thinking of the broader community). At this time, there isn't a way to only use the browsers I configure in the Karma config. We're forced to use PhantomJS if we don't have any parameters or if we specify headless.

Now, when angular-librarian creates a new project for a developer, it sets the browsers property to Chrome and PhantomJS. Default as I've written will run these two browsers if the developers doesn't do anything which fulfills the convention over configuration issue you presented but exposes a different problem with the publish.

I also use TravisCI since it was free with GitHub useage. CI systems usually have a few phases like, checkout, build, test, and "post testing". Typically, after testing succeeds, you may want to publish your library and that's where the "post testing" phase comes in. The current setup will cause another test to occur just because I'm publishing. It seems like some of the decisions made for the angular-librarian workflow were based on a developers doing everything from their computer. I'm not actually saying that was how it was done but I had to heavily modify some of the files angular-librarian generated to get it to allow the CI server to work as I expected it to.

angular-librarian should ideally make it easy to handle both situations. Since I'm familiar with the CI half, I've been trying to add that functionality to it. Maybe we should look at these changes more holistically to see how it fits in overall.

Yeah, I think, perhaps, we can make it so that if no browsers are provided, either via the command line or by a config file, we fallback to the defaults of PhantomJS or Chrome. Does that sound ok to you?

Ok just so I'm not making assumptions about what you're saying I'm going to paraphrase what I believe you are saying. angular-librarian will still by default place Chrome and PhantomJS in the browsers property. The default testing option of not specifying any parameters will provide the new functionality of this PR, which is to run the browsers specified in the karma file. But if the developer has purposefully removed all browsers from the karma file, the tool will attempt to at the minimum use PhantomJS or Chrome. Is my assessment correct or did I miss anything? If this is correct, I'd like to suggest just using PhantomJS just in case a CI server is involved, since CI servers generally don't have browsers installed.

Glad you asked to clarify! Here's the scenarios I'm thinking of...(note that ngl test is the same as running npm test)

User runs the following:

npm test

Since no browser overrides have been provided, it falls back to PhantomJS

User provides a command-line argument for ChromeHeadless:

npm test --browsers=ChromeHeadless

Since the custom config provides an override, tests are run with ChromeHeadless

User provides a command-line argument for Chrome & Firefox for a watch

npm test watch --browsers=Chrome,Firefox

Both Chrome & Firefox are opened by Karma and files are watched

However, does this cover your use case for CI testing? I'm wondering if there needs to be a way of specifying browsers to fallback to as either a custom config or on project init.

Also, I really want to thank you for the effort you've already put in between discussions and actual code!

Yeah, no problem but I have to admit helping is slightly selfish on my part. I'm hoping that the library can get to the point where I won't have to do a lot of work every time there's an update. :) lol.

Anyway, I think you're over thinking this a bit. The karma file should be the source of truth for configuration. If a developer is running Karma tests, there's supposed to be browsers there for the tests to use. The override is for when you want to use a sub or super set of those browsers. In my specific case, my karma file has 10 browsers for SauceLabs testing. When I run the testing, I want those browsers to be used and not added to or overridden with PhantomJS.

On the other hand, if the karma file itself specifies no browsers by the time the library is ready to start Karma THEN we fallback to PhantomJS because a Karma test with no browser is completely useless.

Ok, I'm ok with the proposal if there's no browsers specified always falling back to PhantomJS.

gonzofish · 2018-02-25T00:55:49Z

commands/initial/templates/tasks/test.js

+				let browsers = arg.match(/\w+=([\w,]*)/i);
+				options.browsers = (browsers && !options.browsers) ? browsers[1].split(',') : options.browsers;
+				break;
+			//the 'all' option did not modify the browser options and it did not change the watch option.


good point on this one

gonzofish · 2018-02-25T01:03:24Z

commands/initial/templates/tasks/test.js

+
+	args.forEach((arg, index)=>{
+		let value = arg.match(/^(\w+)/)[1];
+		switch(value){	//there are probably libraries that parse cmd line arguments...


I wanted to mention, Librarian has its own focused utility for handling options:
https://github.com/gonzofish/angular-librarian/blob/master/tools/utilities/options.js

Here's an example of how its used:
https://github.com/gonzofish/angular-librarian/blob/master/commands/component/index.js#L20

Again, totally understand there are libraries that do this, but when I started Librarian I was just having fun and rolled my own--the options.js file is fully tested, though:
https://github.com/gonzofish/angular-librarian/blob/master/test/tools/utilities/options.spec.js

gonzofish · 2018-02-25T01:12:19Z

commands/initial/templates/tasks/test.js

+function parseOptions(args){
+	let options = {};
+
+	args.forEach((arg, index)=>{


The changes that would help here are to just follow the coding conventions/style I've been using.

Use args.reduce((options, arg) => { /* ...code... */ }, {}); instead of a .forEach

Single quotes instead of double quotes

Use const instead of let for variables that won't change

Admittedly, I need to do a better job of managing the styling via ESLint or Prettier, but I never got around to it

gonzofish · 2018-02-25T01:15:26Z

commands/initial/templates/tasks/test.js

@@ -62,5 +57,7 @@ const getAllConfig = (watch) => ({
 module.exports = run;

 if (!module.parent) {
-    run(process.argv[2]);
+	//skip the first two args (exe and script) and grab all options that start with a 'word'
+	let optionArgs = process.argv.filter((value, index) => index > 1 && value.match(/^\w+/));	


You might be able to combine this with options.js, not completely sure though

I'm not sure if you saw my comments about options.js but since the original code did not use it, I would like to update that in a separate PR. The reason for this is because as I was implementing, the code got messier and I'm sure this wasn't the intention for using it. I'm probably looking at it from the wrong perspective and would like to understand the intent behind its design but I also feel that it shouldn't hold this PR up while we discuss. Do you agree with this?

SirDarquan · 2018-03-03T13:56:40Z

The fallback system is in place and the reduce code is as well.

SirDarquan · 2018-03-05T00:03:05Z

After further investigation, none of the changes in this PR moves angular-librarian in the direction of being more robust for developers.What I've found to lead me to this conclusion is that the code in tasks/test.js happens before any of the code that runs in karma.conf.js. This means that it's not possible to know if a developer has configured one or more browsers for testing at the point where we're trying to ensure they have at least one. Furthermore, Karma respects all values added to the config object that is passed to the Karma server, meaning it's not possible to override that value with something from the config file generation.

Now, I completely understand what you're trying to do with the request to ensure PhantomJS is there if no browsers are defined. You want all parts of the system to work in a predictable way and to add conveniences that I developer might have to do. So my question to you is, were you attempting to make such an opinionated library or was the intent to make more of a development tool and it just happens to reflect a really specific workflow? The current workflow requires me to update the tasks/test.js and karma.conf.js files for my library to work the way I need it to for a CI server. Naturally, I also have to be aware of changes to those files whenever an angular-librarian update occurs. Maybe my project is the weird one and the workflow you created is what most need. In that case, the work I need to do should be my burden alone.

In any case, I still like the library and would like to help but I'm not sure how to do so with the current constraints. So I believe that this PR should be closed and not reintegrated.

gonzofish · 2018-03-15T16:00:43Z

It definitely was intended to be more opinionated, but as flexible as possible. I still think there may be a way to achieve what you're looking for. If I find some time I'll try to implement...but my personally life is (still) a bit hectic right now

SirDarquan added 2 commits February 4, 2018 15:36

updated testing switches to all for watch, headless and browsers over…

227ec7a

…ride options

updated README explaining changes

5df631f

gonzofish reviewed Feb 25, 2018

View reviewed changes

gonzofish suggested changes Feb 25, 2018

View reviewed changes

SirDarquan added 2 commits February 25, 2018 12:33

changed forEach to reduce

ffa1b47

created browser fallback incase developer misconfigures karma

61165f1

SirDarquan closed this Mar 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update testing commands #97

Update testing commands #97

SirDarquan commented Feb 4, 2018

gonzofish Feb 25, 2018

gonzofish Feb 25, 2018

SirDarquan Feb 25, 2018

SirDarquan Feb 25, 2018

gonzofish Feb 26, 2018

SirDarquan Feb 26, 2018

gonzofish left a comment

gonzofish Feb 25, 2018

SirDarquan Feb 25, 2018

gonzofish Feb 26, 2018

SirDarquan Feb 26, 2018

gonzofish Feb 26, 2018

gonzofish Feb 26, 2018

SirDarquan Feb 27, 2018

gonzofish Mar 2, 2018

gonzofish Feb 25, 2018

gonzofish Feb 25, 2018

gonzofish Feb 25, 2018

gonzofish Feb 25, 2018

SirDarquan Mar 3, 2018

SirDarquan commented Mar 3, 2018

SirDarquan commented Mar 5, 2018

gonzofish commented Mar 15, 2018

Update testing commands #97

Update testing commands #97

Conversation

SirDarquan commented Feb 4, 2018

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

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

SirDarquan commented Mar 3, 2018

SirDarquan commented Mar 5, 2018

gonzofish commented Mar 15, 2018