concord-consortium / codap

CODAP (Common Online Data Analysis Platform)
MIT License
94 stars 38 forks source link

Make Date a valid IValueType, refactor how dates are handled in formulas and case table #1370

Closed pjanik closed 1 month ago

pjanik commented 1 month ago

This PR aims to fix some design issues that were causing the following problem:

seconds(date(2020, 5, 14, 5, 4, 3)) or seconds(AnotherAttrWithFormattedDate) was returning 0 because I used the formatDate utility function, which formats the date to a locale string often without seconds, too early in the whole processing chain. The problem was that Date was not a first-class citizen type and was always stringified, and some data was lost.

Here’s what I did to fix it:

@kswenson, does this make sense to you? Another approach might be to have a "real" Date storage in the attribute, similar to strings and numbers. However, I'm not sure where and when even these numeric values are used, as often various helpers work with string values only (e.g. useColumns). And I'm concerned this would be quite a significant change. Also, you might have a better sense of how to implement this approach than I do. This is always an option for the future, as the current implementation does not lock us into it in any way.

Also, I did not update attribute.toNumeric to convert date strings to epoch seconds/milliseconds because I don't see a use case for it yet, and it would cause some performance overhead if we had to parse each string passed there, hoping it's a date.

cypress[bot] commented 1 month ago

1 failed and 5 flaky tests on run #3816 ↗︎

1 197 28 0 Flakiness 5

Details:

Merge pull request #1370 from concord-consortium/187799270-IValueType-Date
Project: codap-v3 Commit: 86868a3a53
Status: Failed Duration: 10:37 💡
Started: Jul 24, 2024 10:45 AM Ended: Jul 24, 2024 10:55 AM
Failed  cypress/e2e/graph.spec.ts • 1 failed test View Output
Test Artifacts
Graph UI > graph inspector panel > change points in table and check for autoscale Test Replay Screenshots
Flakiness  table.spec.ts • 2 flaky tests View Output
Test Artifacts
case table ui > case table Inspector menu options > check hide/show attribute from inspector menu Test Replay Screenshots
case table ui > table cell editing > edits cells Test Replay Screenshots
Flakiness  slider.spec.ts • 2 flaky tests View Output
Test Artifacts
Slider UI > updates variable name Test Replay Screenshots
Slider UI > adds new variable value Test Replay Screenshots
Flakiness  toolbar.spec.ts • 1 flaky test View Output
Test Artifacts
codap toolbar > will open a slider Test Replay Screenshots

Review all test suite changes for PR #1370 ↗︎

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.52%. Comparing base (e2edec3) to head (7cdb8d9). Report is 11 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1370 +/- ## ========================================== - Coverage 85.81% 85.52% -0.30% ========================================== Files 521 521 Lines 25210 25266 +56 Branches 6856 6449 -407 ========================================== - Hits 21635 21609 -26 - Misses 3421 3503 +82 Partials 154 154 ``` | [Flag](https://app.codecov.io/gh/concord-consortium/codap/pull/1370/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | Coverage Δ | | |---|---|---| | [cypress](https://app.codecov.io/gh/concord-consortium/codap/pull/1370/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `71.75% <55.55%> (-1.29%)` | :arrow_down: | | [jest](https://app.codecov.io/gh/concord-consortium/codap/pull/1370/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium) | `53.48% <100.00%> (+0.09%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=concord-consortium#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.