This PR builds and tests `@appium/support` without Gulp. Much like the changes to `appium`:
- Test files reorganized and renamed
- Tests run via `mocha` proper using `@babel/register` (no build needed)
- Build run via `@babel/cli`
Since multiple packages will be using this file, it should probably just live here.
We will eventually be able to remove the `--require ../../test/setup-babel.js` from the testing scripts once we've gotten off of gulp entirely (it can be referenced in `.mocharc.js` instead)
- add note about argparse tomfoolery
- refactor `list` command impl for better types
- rewrite exception in `driver-config`
- add `hasDriver`, `hasPlugin`, and small refactors in `manifest`
- add note about args in `grid-register`
- remove invalid comment in `main`
- more test coverage for `manifest`
simplified some stuff that was giving me problems.
instead of doing a try..await..catch and calling `fail()` if the catch block wasn't called, just use `sinon-chai`'s `.should.eventually.be.rejectedWith(ErrorClass, /message/)`
depending on the order in which this test file is run versus others, it's possible that some will fail because of global logging weirdness. but normally this doesn't happen... keep an eye on it!
This addition was prompted by #16076, which I was loathe to merge until I got this work done.
Further:
- changed the signature of `runAppiumRaw` helper to require options (because currying)
- in lieu of an actual plugin for mocha that handles snapshots, added some helper functions to help with comparing `appium`'s output
- revert all of the removals of `@ts-expect-error` I did somewhere else. this was due to a stale TS lang server
* chore: ban direct usage of sinon
Calling `sinon.stub()`, `sinon.mock()` etc., directly is error-prone. Care must be taken to explicitly restore any stub created.
Using a sandbox (via `sinon.createSandbox()`), you don't need to remember to restore everything you fiddled with.
To that end, I've converted all direct `sinon` calls into sandbox calls and **BANNED** direct usage of `sinon` via ESLint.
# Conflicts:
# .eslintrc
* test(base-driver): replace restricted sinon.spy() usage in server-specs.js
* test(support): formatting
This change removes `gulp` and replaces it with plain ol' `babel` and `mocha`.
## `appium`
- No gulp.
- `test/` has been re-organized. All test files now end in the extension `.spec.js`, which is a recognized convention and understood by editors/IDEs. The tests are further split into `unit` and `e2e` subdirs. This makes working with both `babel` and `mocha` easier.
- The tests are _not_ run against the transpiled code; code is now transpiled on-the-fly via `@babel/register`. This only affects `appium`.
- `commands-yml/validator.js` moved to `scripts/parse-yml-commands.js`. It has been rewritten as a CJS module. `commands-yml/` should not contain `.js` files.
- `test/setup.js` is a new test harness specific to Appium, which loads & configures `@babel/register`. Eventually, the contents of this file can be moved into the root monorepo's `test/setup.js`, but the rest of the packages do not need `@babel/register`, so we avoid the overhead.
## `@appium/gulp-plugins`
- Unfortunately, due to the new `npm test` strategy (see below), we cannot use the `nyan` reporter. Or rather, we _can_, but will not see the animation. So the reporter is now set to default to `spec`.
- Modified tests so they didn't overwrite the actual `build` dir with transpiled fixtures, which is what was happening. This was causing failures depending on the transpile/test order.
## All Packages
- All packages now have their own `build`, `test`, `test:e2e` and `dev` (just build + watch mode) npm scripts. This makes running a complete build easier, since each package can now provide its own appropriate command. This is _mostly_ just `gulp once` (where `gulp` was used before), except for `appium`, where `@babel/cli` is used directly.
- This is what happens when `npm test` is run from the monorepo (in order):
1. All packages are built (in parallel) by running `npm run build` in each folder (via `lerna`). See `pretest`
2. All code is linted directly via `eslint`.
3. All _unit tests_ are run w/ `npm test` in each folder, having assumed transpilation already occurred (if necessary).
- `npm run e2e-test` runs the `npm run test:e2e` in each folder. No transpilation occurs! Careful.
- All scripts are expected to live in `packages/*/scripts`, and this directory enforces CJS via ESLint. `postinstall.js` moved, as well as `check-npm-pack-files.js`. `generate-schema-declarations.mjs` moved from the root `scripts/` dir into `packages/appium/scripts/`; it's now CJS instead of ESM.
- I had profiled running various things with `lerna run --parallel`, but it turns out this is not faster. I'm not sure why, but I think it may have something to do with streaming STDOUT/STDERR, or potentially the overhead of running child processes outweighs the time it takes to actually run tests and builds.
- The behavior of `lerna run` now mimicks `lerna exec`, which was "run stuff in serial, streaming the output". The default behavior of `lerna run` is "run stuff in parallel, buffering the output". Buffered output sucks, and streaming the output from all tasks at once creates chaos. To override this behavior, you'll see `--parallel --concurrency=8 --prefix` for tasks that can truly be parallelized, like transpilation. The number 8 is the "maximum", and CI usually won't have more than 2 cores available anyway.
- When run in CI, Mocha runs with `--forbid-only`, which will fail if there's an `.only()` somewhere in the tests. Further, it forces color output.
## Etc.
- Updated Wallaby config, `.gitignore`, `.eslintignore`.
- Sorted `package.json` files
- Removed `watch` and `coverage` scripts which were broken anyway.
chore(appium): fix schema json path issues
- the path to the JSON schema is now correct: `lib/appium-config-schema.json`
- add `log-symbols` to devdeps for niceness. this is already a transitive dep
(where applicable)
- typed and documented some stuff
- added `fs.openFile` which resolves w/a `fsPromises.FileHandle` object. `fs.open` still resolves with a file descriptor. this is, I think, the only explicit conflict between the builtin `fs` module and ours. we should consider deprecating or removing our `fs.open` and using the `FileHandle`-returning impl, because it's a little easier to use. Once we do, we can also remove `fs.close()`, `fs.read()` and `fs.write()`, as those are redundant since they are methods of the `FileHandle` object returned from `fsPromises.open`. A cursory check in the `appium` org suggests that `open` and its ilk are not used except in some test cases.
- assign properties directly to the `fs` object, as this is easier for the type system to understand (rather than iterating over property names and assigning properties in the loop)
- `which` actually returns a `Promise` now and does not need promisification
- added note about future changes to `rmdir`
- fix some types, mocks
- rename `HASHFILE_RELATIVE_PATH` to `PKG_HASHFILE_RELATIVE_PATH`; hopefully that's less opaque
- fixed broken junk in `commands-yml/parse.js` which will conflict with #16459 so idk
- `mkdirp` module is now deprecated and re-exports `fs.mkdirp`, which is new
- fix some types in `fs` module
- fix `walkDir` which did not actually resolve to `null` ever
- Altered strategy for determining `APPIUM_HOME` by using `npm ls` and reading `package.json` if that fails
- rename `isLocalAppiumInstalled` to `hasAppiumDependency` in `env` module
- no longer care about `peerDependencies`; we can add this later if necessary
- consume `pkg-dir` module
- remove `read-pkg-up`
- move `npm` module into `support` (which is the `feat` here, I guess)
- the `NPM` class no longer has internal state, so doesn't really need to be a class, but I didn't want to bother with that refactor. the module exports the class `NPM` and an instance `npm`. creating new instances of `NPM` are useless, but there's no guard against it.
- method signatures in `NPM` class have changed to generally accept a `cwd` string. further, non-optional options have been moved out of an optional options object. 😄
feat(appium): remove NPM module (it went into @appium/support)
- Probably not a breaking change, but it certainly breaks a few things in `appium`, which I addressed.
- Updated for new `NPM` api
- fixed sandboxing issue in `cli-specs`
- update `mocks`
## BREAKING CHANGES
(potentially, anyway)
1. Installation of extensions via `appium` now treats `$APPIUM_HOME` _mostly_ like a project containing a `node_modules` subfolder, with extensions under that. Previously, each extension would have its own folder, with a `node_modules` and empty `package.json`, and finally the extension. The intent was to inhibit deduping and conflicts with any `package.json` files in ancestors of `$APPIUM_HOME`. I showed this was unnecessary because the same thing can be accomplished via the flag `--global-style` for `npm install`. Ultimately, this simplifies the directory structure of `$APPIUM_HOME`.
2. `$APPIUM_HOME/extensions.yaml` now lives at `$APPIUM_HOME/node_modules/.cache/appium/extensions.yaml`. The `node_modules/.cache/<pkg-name>` convention is fairly common for packages having temporary files which need to persist while a module is installed (no more, no less).
3. If the _current working directory_ contains a `package.json` which has `appium` as any sort of (non-`file:`) dependency _or_ `$CWD/node_modules/appium` is a resolvable module, `APPIUM_HOME` defaults to `cwd`.
4. "Install" and "link" lockfiles (see `lib/cli/npm`) have also moved from `$APPIUM_HOME` to `$APPIUM_HOME/node_modules/.cache/appium`.
5. The `runExtensionCommand()` function in the `cli/extension` module no longer accepts a `type` parameter, which can be derived from the `configObject` parameter anyway (redundant)
6. Loading of extensions (i.e. reading `extensions.yaml`) has been moved into its own function, `loadExtensions()` in `extension/index.js`.
7. `getActivePlugins` and `getActiveDrivers` have been moved here from `lib/main`. Changed the signatures of these, since they don't actually need the entire `ParsedArgs` object.
8. `ext-config-io` module, which handles reading/writing `extensions.yaml` has been moved to `extension/manifest.js`.
9. The `installPath` property in `extensions.yaml` has been removed and is unneeded due to 1.
10. `read()` has been removed from `extension-config` and its subclasses.
11. Moved `ReadonlyMap` from `utils` to `schema/schema` module. Probably so I didn't have to stub it in a test.
12. Moved `rootDir` from `utils` to `config`.
13. Removed `drivers` module; moved `findMatchingDriver` to `DriverConfig#findMatchingDriver`. Removed `AppiumDriver#_findMatchingDriver`; calls `DriverConfig#findMatchingDriver` directly instead.
14. Removed `plugins` module; its contents now live in `constants`.
15. Dynamic determination of `APPIUM_HOME` now happens in the `env.resolveAppiumHome()` method of `@appium/support` (introduced in a previous commit). It should not happen anywhere else, because it will be wrong.
16. Because the `lib` dir was becoming rather unwieldy, I moved modules associated with extension loading and config into an `extension` subdir
17. `getParser()` in `cli/parser` is no longer async; the async stuff it was doing moved into `loadExtensions()`. `getParser()` can be used without calling `loadExtensions()` first (no extensions will be loaded)
## Summary
This enables installation of extensions via `npm`; e.g., `npm install appium-xcuitest-driver -D`. Because `appium` usually manages the `extensions.yaml` (the "manifest"), this is not possible when installing modules via other means. To handle this situation, if we have an `appium`-dep-containing `package.json` in `APPIUM_HOME`, a hash is taken of the `package.json`. This hash is checked at runtime; if it has changed, `appium` will scan `node_modules` (_not_ `package.json`; we only care about extensions that are _actually installed_, which is different from how we handle an `appium` dep) for extensions, and the hash will be updated. Extensions are defined as "modules having a `package.json` which contains an `appium` field which is an object of a particular shape". When these are found, they are appended to `extensions.yaml`. In theory, this allows a user to use `appium` _or_ `npm` (at their convenience) to manage extensions.
See the "when appium is a dependency" suite in `test/cli/cli-e2e-specs.js` for more context.
## Notes
- Added a `constants` module, which provides a centralized place for various constants. This makes finding constants less cognitively demanding. 🙂 You may not like this; if you don't, the constants can be moved to more appropriate places.
- Added `extension/package-changed` module which is used to determine if a `package.json` has changed
- Many types are added/changed throughout. See especially `extension/manifest` for many type definitions involving the shape of `extensions.yaml` and the `appium` prop of extensions' `package.json`.
- Reorganized some tests based on structure of `lib`.
- Split `extension-config-specs.js` into `driver-config-specs.js` and `plugin-config-specs.js`.
- Many refactors in now-fully-typed `cli-e2e-specs`; introduced some more helpers in `test/cli/cli-helpers.js` and used these. Makes calling out to the `appium` executable from E2E tests a little easier.
- Introduced `test/extension/mocks` for DRY mocks, since many of the unit tests here now use the same ones.
- Consolidated fixtures into `test/fixtures`. Fixtures can always be found via the `resolveFixture()` method of `test/helpers.js`.
## TODO (before this lands)
- Instead of using `$CWD` for `APPIUM_HOME` if `appium` is a dep, we probably want to use `find-pkg-up` or something. This should be okay, since if we find a `package.json` and `appium` _isn't_ a dep, we will just be using the default `APPIUM_HOME`.
- We _may_ also need to search `package.json` for known extensions, since it's possible to use them standalone. However, I'm not sure if this use case needs `APPIUM_HOME` at all?
chore: vscode stuff
- change default build task to `dev`, which just transpiles in the background
- updated `test - current file` launch config for better debugging
chore(appium): peer review refactors
- The old class name of `Manifest` used to be `ExtConfigIO`, but I forgot to update these variable names. `io` is now `manifest`, wherever used
- Created a more clear separation between the `create()` and `getInstance()` static methods of `DriverConfig`/`PluginConfig`. `create()` should _only_ be used for instantiation, and `getInstance()` should _only_ be used if we have already instantiated the object with an associated `Manifest`. `create()` now _throws_ if the `Manifest` already has an association in place. The reason this is split into two functions instead of just using `_.memoize()` is because of the options in the second parameter (`extData`, `logFn`), which would require the _same configuration object_ in order to return the already-created instance. This is not a problem in `Manifest`, where the factory only accepts a single parameter.
- The `_instances` static method of `PluginConfig`/`DriverConfig` is now a `WeakMap`, keyed on a `Manifest` instance instead of the `appiumHome` string. This makes the relationship between them more explicit.
- Fixed some formatting
- Fixed type issues in schema tests, `grid-register` and its tests
- Made the constructor of `Manifest` private, created a memoized static method `getInstance()`; removed `getManifestInstance()` export.
- Refactored `let`-destructuring to the fancy new syntax I learned from @mykola-mokhnach
- Explained the meaning of `CONFIG_DATA_DRIVER_KEY` and `CONFIG_DATA_PLUGIN_KEY` in `manifest` module
- Replaced unnecessary use of `null` with `undefined` in `Manifest` class
- Removed runtime type check from `packageDidChange()`
- Had some fun renaming `ReadonlyMap` in `schema` module
- `env` module exposes functions for determining `APPIUM_HOME`, where various files should live, etc., by examining the runtime environment
- fixed some functions in `fs` which unnecessarily used `this` (which prevented dereferencing)
- added `walk()` to `fs` which just exposes `klaw` (since `walkDir` has very limited behavior). this enables `walk()` to be used as an async iterator, which is handy.
- fixed a bunch of types in the `util` module. involved some actual code changes, but the original code only worked by chance.
- removed `strict` option from `util.coerceVersion()`, since the option isn't actually used anywhere. this should be considered a breaking change if some module somewhere _else_ uses it and we consider it a public API. I'm pretty sure that's not the case, so.
- Upgrade TS to `next` tag. It's "safe" to use using bleeding-edge TS here since we don't actually use it to compile anything
- Add a bunch of missing types: `find-root`, `klaw`, `lockfile`, `mv`, `ncp`, `rimraf`
Renovate Bot