search

arqex / react-datetime

A lightweight but complete datetime picker react component.
1.99k stars 873 forks source link

Tests fail with latest master #780

Open johnhunter opened 3 years ago

johnhunter commented 3 years ago

I'm Submitting a ...

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

Steps to Reproduce

  1. git clone git@github.com: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 commented 3 years ago

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

johnhunter commented 2 years ago

Tests still fail when checking out master

Steps

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.

arqex commented 2 years ago

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.

johnhunter commented 2 years ago

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.