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

adopt Ava default files glob #8273

Closed
turadg opened this issue Aug 29, 2023 · 2 comments · Fixed by #8653 or #9352
Closed

adopt Ava default files glob #8273

turadg opened this issue Aug 29, 2023 · 2 comments · Fixed by #8653 or #9352
Labels
enhancement New feature or request tooling repo-wide infrastructure

Comments

@turadg
Copy link
Member

turadg commented Aug 29, 2023

What is the Problem Being Solved?

Our package.json has an "ava": section that configures a custom "files" glob. There are two problems with this:

  1. It requires that every package.json repeat the same custom config. Customizing tool defaults should have a high bar because of the maintenance cost they incur.
  2. The naming scheme chosen puts "test" at the front of what is obviously a test from its path context. To run a particular test from the CLI requires typing "test" three time to pick one: yarn test test/test-something.js. With Ava's default it would be yarn test test/something.test.js and typing the "so" can autocomplete.

Description of the Design

Remove all the custom globs.
Rename test files to .test.js.

Security Considerations

none

Scaling Considerations

none

Test Plan

Before closing this ticket, check that all open PRs don't have new test files that would be ignored upon merge.

Upgrade Considerations

none

@erights
Copy link
Member

erights commented Dec 23, 2023

This should only be fixed for agoric-sdk (this issue) in coordination with fixing it for endo (endojs/endo#1913)

See #8653 (review)

@mergify mergify bot closed this as completed in #8653 May 9, 2024
mergify bot added a commit that referenced this issue May 9, 2024
closes: #8273

## Description

Adds a script to migrate a package's file glob to the Ava defaults. See
script docs and #8273 for motivation. It uses
https://github.com/google/zx because I was banging my head on Bash
options and version inconsistencies.

The main thing to review is whether this change is good. Then the code
in the first commit. The subsequent commits just are the code running
over all the packages. When this PR is approved I'll rebase and re-run
the batch to make all the migration commits anew.

Once this is in SDK, we can do the same in other Agoric repos. 

### Security Considerations

n/a
### Scaling Considerations

n/a
### Documentation Considerations

If anything, less need for documentation because it aligns with how Ava
works by default.

### Testing Considerations

To run the script yourself, ensure,
```
npm install -g zx
```



### Upgrade Considerations

n/a
@turadg
Copy link
Member Author

turadg commented May 9, 2024

To be closed in June, when

  • files property removed, so only default is available
  • check that all open PRs don't have new test files that would be ignored upon merge.

@turadg turadg reopened this May 9, 2024
turadg added a commit to endojs/endo that referenced this issue May 9, 2024
Refs: Agoric/agoric-sdk#8273

## Description

Akin to Agoric/agoric-sdk#8653

### Security Considerations

none

### Scaling Considerations

none

### Documentation Considerations

reduces need to document differences

### Testing Considerations

CI

### Compatibility Considerations

Developers will need to know to make the new files. This globs for both
old and new so no tests are accidentally omitted from CI.
Agoric/agoric-sdk#8273 will be closed by
dropping the custom glob and ensuring no old names remain.

### Upgrade Considerations

none
@mergify mergify bot closed this as completed in #9352 May 28, 2024
mergify bot added a commit that referenced this issue May 28, 2024
closes: #8273

## Description

#8653 adopted the new glob but
left the old one so any in-flight PRs with new tests wouldn't have them
ignored.

It's been a month so any should be migrated by now. This removes the old
glob.

It doesn't remove the `"files"` option entirely, because the default
glob is everything under `test` and we have files there that aren't
tests. Ava warns they're not tests and and eslint errors when tests
import from them. We could go fully to the default glob by using an
underscore prefixes on files in `test` that aren't tests. I don't think
we need to do that. This change already meets the goal of matching the
DX of Ava in other repos. (Test filenames are indicated by a `.test.js`
suffix)

### Security Considerations
n/a


### Scaling Considerations
n/a

### Documentation Considerations

reduces need to document

### Testing Considerations

no older test files in master (`find packages -name 'test-*' |grep
'test/'`)

### Upgrade Considerations

n/a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants