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

Refactor formatTime into util class #3073

Merged
merged 11 commits into from
Apr 7, 2023

Conversation

rejas
Copy link
Collaborator

@rejas rejas commented Mar 25, 2023

While looking at #3070 I noticed that the weather and clock module do some formatTime stuff, so why not use a common function for that?

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2023

Codecov Report

Merging #3073 (cc9fc44) into develop (2c7beea) will increase coverage by 0.43%.
The diff coverage is 91.17%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #3073      +/-   ##
===========================================
+ Coverage    27.51%   27.95%   +0.43%     
===========================================
  Files           52       52              
  Lines        11573    11574       +1     
===========================================
+ Hits          3184     3235      +51     
+ Misses        8389     8339      -50     
Impacted Files Coverage Δ
modules/default/clock/clock.js 0.00% <0.00%> (ø)
modules/default/weather/weather.js 0.00% <0.00%> (ø)
modules/default/utils.js 49.71% <100.00%> (+10.26%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@KristjanESPERANTO KristjanESPERANTO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I haven't tested it, but it looks good to me.

@rejas rejas marked this pull request as draft March 28, 2023 07:49
@rejas rejas force-pushed the util_formattime branch 4 times, most recently from e4e7ca2 to c5ba7b5 Compare April 7, 2023 14:17
@rejas rejas force-pushed the util_formattime branch 3 times, most recently from 08d8821 to 131a982 Compare April 7, 2023 19:39
@rejas rejas force-pushed the util_formattime branch 3 times, most recently from 44084f2 to 276a5a7 Compare April 7, 2023 20:15
... so that tests can rely on using the same timezone
@rejas rejas force-pushed the util_formattime branch from 276a5a7 to c023870 Compare April 7, 2023 20:25
CHANGELOG.md Outdated Show resolved Hide resolved
@rejas rejas marked this pull request as ready for review April 7, 2023 20:51
@rejas
Copy link
Collaborator Author

rejas commented Apr 7, 2023

Running tests with times/dates is a PITA ... Had to add an action to the github-actions for setting the timezone, otherwise the tests would fail here (while being fine locally)...

@khassel
Copy link
Collaborator

khassel commented Apr 7, 2023

Running tests with times/dates is a PITA ... Had to add an action to the github-actions for setting the timezone, otherwise the tests would fail here (while being fine locally)...

hopefully this helps ...

@khassel khassel merged commit 32192d1 into MagicMirrorOrg:develop Apr 7, 2023
@rejas rejas deleted the util_formattime branch April 7, 2023 21:16
@khassel
Copy link
Collaborator

khassel commented Apr 7, 2023

now I understood that only the new tests need the timezone (because these tests are now failing on my side). Is there no way to get these test running without needing a specific timezone? Or move them to the electron section where we can handle such things already?

@khassel
Copy link
Collaborator

khassel commented Apr 7, 2023

found a jest solution, will provide a new PR ...

rejas pushed a commit that referenced this pull request Apr 8, 2023
needed for new formatTime unit tests.

Avoiding ugly TZ setting in github workflows, see
#3073
@khassel khassel mentioned this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants