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

Write more tests #1717

Open
imolorhe opened this issue Oct 17, 2021 · 19 comments
Open

Write more tests #1717

imolorhe opened this issue Oct 17, 2021 · 19 comments

Comments

@imolorhe
Copy link
Collaborator

imolorhe commented Oct 17, 2021

While there are already lots of tests, several components are still lacking proper tests (just the one skeletal generated test). It would be nice to have tests for a lot of such components.

To make it easier to assign/attribute the tests to individual people, here's what we'll do:

Decide on what you want to write tests for
The focus is on packages/altair-app directory, but writing tests for the other files/packages are also acceptable. The tests are divided by the files (so if you're picking up account-dialog component for example, then you're essentially picking up testing for the account-dialog.component.{ts|html} file as a whole, which makes sense). If you only intend to work on a part of a component, you need to make it clear so others know what isn't picked up yet. Note: For now we are not defining a minimum scope for a test task but we change this later depending on how this goes

Aim for at least 70% coverage
While you can choose to write tests for anything in there, the aim is to have test coverage of at least 70% for each individual file. Some things already have tests (pretty much everything has at least one basic test but those don't count much) but the test coverage for most things are currently very low.
To be clear, test coverage is just a basic metric to ensure that we are at least doing some automated checks, but it isn't the defining factor of well tested code. The tests have to be testing the actual business logic, execution flows, etc. So if a test PR is created that just aims to increase the test coverage without actually testing anything substantial, it will not be accepted.
Tip: the test coverage is displayed after the tests run, so you can see the actual coverage by running yarn jest in the packages/altair-app directory. You can also just guesstimate that a file has low coverage if the test file has very little code 🙂

Add a comment about the test task
Once you have decided on the things you want to test, add a comment on this issue about it. That way we can coordinate better if someone already picked it up. We'll create an issue for the task and assign the issue to you, and you can proceed from there.

Completing the task
Create your PR(s) to complete the test. Once that's reviewed and merged and we confirm that the coverage is good enough, we'll close the test issue as completed. Note: if we don't do this promptly, feel free to add a comment on the issue stating that it is done to remind us

Hope this helps make it a better experience!

@OrchiDorchi
Copy link

I am looking forward to work on this issue. Can you provide the tests path and some sample tests?
Thanks.

@imolorhe
Copy link
Collaborator Author

Oh thank you!

The test files are along side the source files in the same directory. For instance, the test file for environment.service.ts is environment.service.spec.ts.

Same for components as well. For example, the test for header.component.ts (which is one of the empty tests) is header.component.spec.ts. An example of a component spec with some valid tests is add-collection-query-dialog.component.spec.ts

@rajpatelbot
Copy link

is it still need test case if yes then please assign me

@imolorhe
Copy link
Collaborator Author

imolorhe commented Feb 7, 2023

Feel free.

@M4X1M4S
Copy link

M4X1M4S commented Feb 16, 2023

still, pending? Assign it to me.

@imolorhe
Copy link
Collaborator Author

To be clear, this is a rolling task and multiple people can work on different parts of it. Feel free to pick it up and I can assign it to you when you have it in progress. 🙂

@ianupam001
Copy link

If it is open still may i contribute

@imolorhe
Copy link
Collaborator Author

Yes it is

adrohan19 added a commit to adrohan19/altair that referenced this issue Apr 21, 2023
@krishh16
Copy link

I would like to contribute.

@imolorhe
Copy link
Collaborator Author

Feel free to do so!

@utkarsh-shrivastav77
Copy link

I would like to contribute

@adityadalwadi1510
Copy link

I would like to contribute.

This was referenced Jul 15, 2023
@TheRecklessDoctor
Copy link

I would like to contribute

@codeinate
Copy link

I'm going to start working on some tests.

@Tiago404
Copy link
Contributor

I'm also going to work on some tests.

@imolorhe
Copy link
Collaborator Author

imolorhe commented Aug 19, 2023

To make it easier to assign/attribute the tests to individual people, here's what we'll do:

Decide on what you want to write tests for
The focus is on packages/altair-app directory, but writing tests for the other files/packages are also acceptable. The tests are divided by the files (so if you're picking up account-dialog component for example, then you're essentially picking up testing for the account-dialog.component.{ts|html} file as a whole, which makes sense). If you only intend to work on a part of a component, you need to make it clear so others know what isn't picked up yet. Note: For now we are not defining a minimum scope for a test task but we change this later depending on how this goes

Aim for at least 70% coverage
While you can choose to write tests for anything in there, the aim is to have test coverage of at least 70% for each individual file. Some things already have tests (pretty much everything has at least one basic test but those don't count much) but the test coverage for most things are currently very low.
To be clear, test coverage is just a basic metric to ensure that we are at least doing some automated checks, but it isn't the defining factor of well tested code. The tests have to be testing the actual business logic, execution flows, etc. So if a test PR is created that just aims to increase the test coverage without actually testing anything substantial, it will not be accepted.
Tip: the test coverage is displayed after the tests run, so you can see the actual coverage by running yarn jest in the packages/altair-app directory. You can also just guesstimate that a file has low coverage if the test file has very little code 🙂

Add a comment about the test task
Once you have decided on the things you want to test, add a comment on this issue about it. That way we can coordinate better if someone already picked it up. We'll create an issue for the task and assign the issue to you, and you can proceed from there.

Completing the task
Create your PR(s) to complete the test. Once that's reviewed and merged and we confirm that the coverage is good enough, we'll close the test issue as completed. Note: if we don't do this promptly, feel free to add a comment on the issue stating that it is done to remind us

Hope this helps make it a better experience!

@Nishant-Nagururu
Copy link
Contributor

I'm writing test cases for the action_bar component in Altair-app. I wrote tests for all the event emitters to make sure that those go off properly.

@austinluk
Copy link

Hello! I want to contribute, is there any components that need to be tested?

@austinluk
Copy link

Is there still anything I can test? if yes then please assign me.

Thanks!

@akhilk2802
Copy link

Can i work on this ? please assign, Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests