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

unit tests feedback #35

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

chienleng
Copy link
Member

No description provided.

Copy link
Member Author

@chienleng chienleng left a comment

Choose a reason for hiding this comment

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

@Phuc-Hong-Tran see comments. I also noticed you are not using eslint or prettier in your code editor. Either run the lint and format before commit or I can setup pre-commit hook to make things easier.

tests/.svelte-kit/ambient.d.ts Show resolved Hide resolved
tests/utils/Statistic/interpolate-data.test.ts Outdated Show resolved Hide resolved
tests/utils/Statistic/interpolate-data.test.ts Outdated Show resolved Hide resolved
tests/utils/Statistic/min-intervals.test.js Outdated Show resolved Hide resolved
import getMinInterval from '$lib/utils/Statistic/min-interval';
import data from './min-intervals-data.json' assert { type: 'json' };

describe("Find smallest interval", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

structurally it feels like it should be:
Describe Find smallest interval

  • test should return smallest interval without provided StatType (default to history)
  • test should return smallest interval between forecasts
  • test should return smallest interval between histories

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant putting the three tests inside one describe.

Copy link

cloudflare-workers-and-pages bot commented Aug 10, 2024

Deploying openelectricity with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5be80be
Status: ✅  Deploy successful!
Preview URL: https://8c8a648a.opennem-app.pages.dev
Branch Preview URL: https://32-adding-unit-tests-for-sta.opennem-app.pages.dev

View logs

tests/.svelte-kit/ambient.d.ts Show resolved Hide resolved
import getMinInterval from '$lib/utils/Statistic/min-interval';
import data from './min-intervals-data.json' assert { type: 'json' };

describe("Find smallest interval", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I meant putting the three tests inside one describe.


const loadFts = ['exports', 'battery_charging', 'pumps'];

describe("Test ", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

please describe the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that, this one is still in-progress

@jamescrowley
Copy link

@Phuc-Hong-Tran @chienleng I assume this is addressing #32? Happy to help get this over the line if useful. Let me know.

@jamescrowley
Copy link

@chienleng I'll pick up in the next few days and polish up - @Phuc-Hong-Tran just ping a note here if you want to jump back in, if I don't hear anything I'll assume good to proceed.

@chienleng
Copy link
Member Author

thanks @jamescrowley. I think there are some updates to the functions since this PR was started so might want to merge from main branch before continuing.

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.

3 participants