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

Update testing commands #97

Closed
wants to merge 4 commits into from
Closed

Update testing commands #97

wants to merge 4 commits into from

Conversation

SirDarquan
Copy link

fixes #96


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

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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.

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.

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!

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

  1. User runs the following:

    npm test

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

  2. User provides a command-line argument for ChromeHeadless:

    npm test --browsers=ChromeHeadless

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

good point on this one


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

Choose a reason for hiding this comment

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

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

function parseOptions(args){
let options = {};

args.forEach((arg, index)=>{
Copy link
Owner

Choose a reason for hiding this comment

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

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

  1. Use args.reduce((options, arg) => { /* ...code... */ }, {}); instead of a .forEach
  2. Single quotes instead of double quotes
  3. 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

@@ -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+/));
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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

@SirDarquan
Copy link
Author

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.

@SirDarquan SirDarquan closed this Mar 5, 2018
@gonzofish
Copy link
Owner

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

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.

Watching tests shouldn't restrict browser choices
2 participants