Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v3.0.0 #3

Closed
1 of 10 tasks
Alhadis opened this issue Mar 11, 2017 · 10 comments
Closed
1 of 10 tasks

v3.0.0 #3

Alhadis opened this issue Mar 11, 2017 · 10 comments

Comments

@Alhadis
Copy link
Owner

Alhadis commented Mar 11, 2017

Literally everything about this module's API and documentation needs improvement. Options aren't obvious, they're weirdly named, and sloppily detailed. The worst offence, however, stems from how unwieldy the most rudimentary options are to configure: which is the exact opposite of how I envisioned this project.

Of course, this isn't surprising: everything was developed as a brisk diversion from file-icons, from which this spec-runner originally sprang from (and was developed for). Everything was either rushed or marked with a mental memo: "redo this later". Little has improved other than features hurriedly added when they were needed...

So. Time to pen a to-do list; and having my rambling words in cold writing limits my excuses to forget:

API (last edited March 2019)

  • Give options shorter, more obvious names
  • Use --disable- as a prefix instead of --no-
    Actually, that'd be inconsistent with Mocha 6/yargs-parser.
  • Allow environment-specific Mocha options to be set in mocha.interactive.opts and mocha.headless.opts files, in addition to the usual mocha.opts
    No longer an issue now that .mocharc.js can programmatically set options based on globals. 👍
  • Add support for configuring Atom-Mocha using --reporter-options option (so mocha can run tests natively alongside Atom in the same spec folder)
    Mocha v6 no longer complains if a foreign setting was picked up in a config file. 🎉 Another issue solved!
  • Offer an easy/obvious way to specify the main spec directory.
    We'll just call the option spec, which is exactly what Mocha uses (although apparently informally? Running mocha --spec ~/some/other/directory/tests works, but not when run with mocha --specs, bringing me to believe this is canon (we'll support specs as a non-standard alias though).

UX

  • Default Reporter: .
    • Add unintrusive progress-bar
      • Probably better to make the reporter run in a special "minimal" mode with only a progress-bar, similar to how it does when reporter is set to dot in interactive mode.
      • An alternative feature might be to display a percentage in the spec-runner window's title(?)
    • Add a freaking cancel button
      Plus, let's use a cute menu accelerator which coincides with the user's SIGINT keybinding.
    • Stop auto-scrolling if user scrolls up while specs are running
    • Use IPC/Electron sorcery to temporarily override settings when rerunning specs.
      • Rerun with/without --bail
      • Set --grep/--fgrep temporarily, but that'll require a UI component because Electron gave window.prompt the shaft, lol
    • Remove autoIt/when in favour of mocha-when module

Deletions / Changes

Nuke postinstall hook. Adding atomTestRunner to one's package.json should be done manually by developers.

Remove all reporter-specific settings from top-level configs. Reporter-level tweaking should be done with JavaScript or CSS.

🔥 Delete: Reason:
autoIt Too specific. Replace with mocha-when, which will also handle the when global.
autoScroll Always auto-scroll. Improve behaviour to allow users to break away from bottom edge if they choose
clipPaths: Always enable. Controlled by a higher-level setting to disable feedback tampering
escapeHTML Always escape. If authors want to see HTML elements, they should use attachToDOM
flipStack Pointless. Absurd setting with zero practical use.
hidePending Redundant. This is what CSS is for.
hideStatBar Redundant. This is what CSS is for.
opacity Redundant. This is what CSS is for.
linkPaths Always enable: Behaviour refined using JS
slide Always enable: Behaviour refined using JS
stackFilter Always enable: Use modest defaults. Hide Mocha/Chai frames from traces. Use CSS to filter frames.
title Redundant. If users care that much about the test-runner window's title, they can set it themselves in JS
minimal Pointless. Vague and ties in too much with --reporter type, which should be explicit anyway
formatCode Always enable. Developers can hack it in their runner's JS if they need to, and it's not like it's intrusive anyway
css / js Keep. They still make sense, even if js should kinda be ditched in favour of variable-length --require options…
@cgalvarez
Copy link
Contributor

cgalvarez commented Feb 10, 2018

I have my first contribution to this.

I'm setting right now a reliable way to set entries on the package.json file of the project requiring mine. As I noticed yours do it (sets the entry "atomTestRunner"), I digged into your code at bin/post-install.js, and there found something that broke my local workflow.

You assume here that the root of the project requiring the package is two steps above (the folder of your project and node_modules).

I have installed my package under development in a local folder and required it as "my-pkg": "file:path/to/local/pkg" inside my package.json, so that approach doesn't work for me. Maybe linking the project works, maybe not, but I've found that on the postinstall hook there is an environment var set which directly points to the project root: INIT_CWD. So all that I need to do is using process.env.INIT_CWD.

Thought it may be of interest to you! 😉

EDIT: process.cwd() points to my-pkg folder, indeed, as you assumed.

@Alhadis
Copy link
Owner Author

Alhadis commented Feb 11, 2018

Honestly, I regret even adding that hook in the first place. That too should go, for the very reason you've just explained. Authors shouldn't need hand-holding to add an entry to their package.json file, after all. Stupid of me.

@cgalvarez
Copy link
Contributor

Allow environment-specific Mocha options to be set in mocha.interactive.opts and mocha.headless.opts files, in addition to the usual mocha.opts.

Mocha headless implies it won't open the report window when running inside Atom? I'm planning another Atom package to act as test runner, and I would like to run my AtomMocha specs in the background, dynamically on any detected change, instead of firing them manually (with no additional open window). Do you think it would be possible/feasible to integrate this behavior in your new on-planning version?

Maybe the easiest approach would be executing specs as atom --test specs/*.js/apm test specs/ in a detached child process...

@Alhadis
Copy link
Owner Author

Alhadis commented Feb 20, 2018

Hey @cgalvarez,

  • Interactive means the spec-runner has been launched from inside Atom, typically by running window:run-package-specs.
  • Headless means the spec-runner has been launched by running atom --test via terminal or CI.

I'm afraid I have little control over the latter, because this is how Atom's test-runner API is setup. It doesn't even allow arbitrary options to be passed to the test-runner, meaning I can't simply do atom --test --grep foo the way I could do mocha --grep foo or whatever.

Your best bet is to run this as a detached process, as you said:

λ atom --test specs/*.js 2>&1 >/path/to/test.log &

Yes, this is a serious pain-in-the-ass, but then again, Atom has never been very friendly to command-line users. 😞

@cgalvarez
Copy link
Contributor

cgalvarez commented Mar 7, 2018

Hi @Alhadis ! More insights for this:

  • Use cosmiconfig for options. It's very flexible and allows for config file written in json/yaml, in addition to config in package.json. It's what I'm using in atom-coverage.
  • Configure the project to use semantic-release, so you can forget about manually publishing new versions (following semver).
  • Make some helpers and provide them globally on each test, pretty much like Jasmine's waitsFor()/runs() and Atom's waitsForPromise(). Mocha has out-of-the-box support for promises, but does not provide any function like those ones to wait until a condition is satisfied. It's what I've commented here, but thinking about it better I think that it should be provided by the test runner (Atom-Mocha), not the coverage tooling.

Regarding the development of the new version... why not to open a new dev branch which other contributors could push to (like me!)? This approach is the one I'm currently using. Pushing all changes to that dev branch, and finally merge into master when polished and ready.

@Alhadis
Copy link
Owner Author

Alhadis commented Mar 7, 2018

Use cosmiconfig for options.

The only dependencies I add are those which are strictly necessary.

Configure the project to use semantic-release

I prefer to audit and document releases by hand. Moreover, semantic-release follows different commit message conventions to the ones I enforce in my own projects.

Mocha has out-of-the-box support for promises, but does not provide any function like those ones to wait until a condition is satisfied

Good idea. 👍 I even wrote a function to do that quite recently, when I was refactoring file-icons's test-suite. See Alhadis/Utils@fd54cca.

Regarding the development of the new version... why not to open a new dev branch which other contributors could push to (like me!)?

I prefer working off master directly. 😉 For work on upcoming versions, I tend to create a temporary branch called v2.0.0 (or whatever) and then squash-merge it into master when ready.

You are of course perfectly welcome to start working on the next version, but please be sure to read this first. 😉

@cgalvarez
Copy link
Contributor

Good idea. I even wrote a function to do that quite recently, when I was refactoring file-icons's test-suite. See Alhadis/Utils@fd54cca.

Me too (yesterday), although I reject the promise on timeout instead of throwing.

I prefer working off master directly. wink For work on upcoming versions, I tend to create a temporary branch called v2.0.0 (or whatever) and then squash-merge it into master when ready.

That's exactly what I was (unsuccessfully) trying to say 😆. I suppose rewriting the package could be done in a separate branch and will imply releasing a new version, so you can review it and ensure the whole checklist in this issue is completed before merging.

You are of course perfectly welcome to start working on the next version, but please be sure to read this first.

Well, then I think my first contribution to the next version will be adding this to a Contributing section in the README.md for every upcoming contributor 😛 . I saw the .editorconfig in the project, because I use them with my projects too, but didn't know about your contributing requirements.

Alhadis added a commit that referenced this issue Mar 21, 2018
Atom v1.25.0 changed how user configs were updated on disk, subsequently
removing some properties which we were relying on to find `config.cson`.
This is causing `loadEditorSettings` to raise an exception at startup.

Since this feature was headed for the incinerator anyway, we may as well
cull it early (it was a stupid idea to begin with, among many others yet
to be obliterated).

See #3.
@Alhadis
Copy link
Owner Author

Alhadis commented Dec 21, 2018

Use cosmiconfig for options. It's very flexible and allows for config file written in json/yaml, in addition to config in package.json

Looks like support for JSON/YAML configs is coming anyway; Mocha v6.0 is due to include .mocharc.{json,yml,js} support, and has actually started to deprecate mocha.opts. 😀 Glad I didn't rush into this, because I was about to push users into using mocha.opts over package.json... which I didn't want to do, but felt it was more consistent with how mocha works.

@Alhadis
Copy link
Owner Author

Alhadis commented Feb 28, 2019

@cgalvarez Good news, I've added support for Mocha 6's new .mocharc.* files, which make storing on-disk configuration and initialisation a heck of a lot easier. 😅 E.g., you can export a beforeStart handler in your project's .mocharc.js file, and it'll get plugged into this.reporter.beforeStart (which is an undoubtedly cleaner way of doing it. 😉

Let me know what you think, and if you hit any bugs I must've missed... :\

@Alhadis Alhadis pinned this issue Mar 1, 2019
@Alhadis Alhadis changed the title Rehaul this mess v3.0.0 Mar 1, 2019
@Alhadis Alhadis added this to the v3.0.0 milestone Mar 1, 2019
@Alhadis Alhadis unpinned this issue Sep 19, 2019
@Alhadis
Copy link
Owner Author

Alhadis commented Sep 19, 2019

Change of plans. Gonna keep things the way they are, and all future work will be concentrating on decoupling reusable code from Atom-specific projects, as I intend to abandon the editor in favour of one yet-to-be-built.

This means keeping the spiffy TTY-style browser reporter, so it can be used in any browser-based Mocha environment. Not gonna let all that hard work die with Atom's userbase. 👍

@Alhadis Alhadis closed this as completed Sep 19, 2019
@Alhadis Alhadis removed their assignment Sep 19, 2019
@Alhadis Alhadis removed this from the v3.0.0 milestone Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants