react-datetime icon indicating copy to clipboard operation
react-datetime copied to clipboard

Tests fail with latest master

Open johnhunter opened this issue 4 years ago • 4 comments

I'm Submitting a ...

[x] Bug report
[ ] Feature request
[ ] Support request

Steps to Reproduce

  1. git clone [email protected]:arqex/react-datetime.git
  2. cd react-datetime
  3. npm install
  4. npm run test:all

Expected Results

Tests pass

Actual Results

Tests fail

Test Suites: 2 failed, 1 passed, 3 total
Tests:       26 failed, 2 skipped, 121 passed, 149 total
Snapshots:   24 failed, 1 passed, 25 total

Minimal Reproduction of the Problem

Other Information (e.g. stacktraces, related issues, suggestions how to fix)

This is with latest master https://github.com/arqex/react-datetime/commit/7e30d6c20cd864bf8e91bc94e6c3a0ee02864d19 using node version: v14.15.5

The same commit as ran on CI here https://travis-ci.org/github/arqex/react-datetime/builds/747440775 CI runs those tests with node 8 and latest stable (v15.3.0)

One thing that makes it hard to reproduce test results is there is no lock file committed to the project. So there is no way to tell if there are dependency issues causing failures.

I would like to propose:

  • We commit the npm lock file (or yarn if the maintainers prefer)
  • We add Dependabot (or another dependency manager) to the repo. Its free, and makes dependency updating straightforward https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/

I think if we make these changes then it will be easier to contribute back while ensuring code quality with reliable tests.

johnhunter avatar Mar 10 '21 17:03 johnhunter

Looks like this might be fixed with PR https://github.com/arqex/react-datetime/pull/761

johnhunter avatar Mar 10 '21 17:03 johnhunter

Tests still fail when checking out master

Steps

  • Checkout current master https://github.com/arqex/react-datetime/commit/4e880f2167789867d6074a655c18c16717a92e49
  • Run npm install
  • Run npm run test:all

Expected Tests pass

Actual Tests fail

Snapshot Summary
 › 24 snapshots failed from 1 test suite. Inspect your code changes or run `npm test -- -u` to update them.

Test Suites: 2 failed, 1 passed, 3 total
Tests:       26 failed, 2 skipped, 121 passed, 149 total
Snapshots:   24 failed, 1 passed, 25 total

It's difficult to contribute if the master branch does not pass tests.

johnhunter avatar Sep 06 '21 13:09 johnhunter

Sorry about that @johnhunter .

We don't use snapshot testing anymore in this library. They are not in the pre-build hooks or in the deployment process, but you are right that we still have the infrastructure there, and their CLI commands are still in the package.json.

We need to remove them completely at some point.

arqex avatar Sep 06 '21 15:09 arqex

Ok thanks, what test suite should be run when contributing?

The tests.spec.js seems to mostly pass, except two of the UTC date ones. Its hard to tell why because they throw an exception in jest-environment-jsdom-fourteen. Some of the test dependencies are really old so it might be worth merging https://github.com/arqex/react-datetime/pull/761. I'm happy to help out where I can if you are accepting PRs.

johnhunter avatar Sep 06 '21 17:09 johnhunter