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

[🛠️] Add unit tests (jest) #11

Open
TheDanniCraft opened this issue Oct 15, 2024 · 18 comments
Open

[🛠️] Add unit tests (jest) #11

TheDanniCraft opened this issue Oct 15, 2024 · 18 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@TheDanniCraft
Copy link
Owner

Is your feature request related to a problem? Please describe.
Currently, we don’t have automated tests set up, which makes it harder to ensure the code quality and reliability of new features or bug fixes introduced through Pull Requests (PRs). Manually testing each PR can be time-consuming and error-prone.

Describe the solution you'd like
I propose adding Jest as a testing framework to the project. This will allow us to write unit tests that automatically run each time a new PR is opened. These tests will ensure that the code works as expected and reduces the risk of introducing bugs in the codebase. The tests should run via a continuous integration pipeline (like GitHub Actions) to ensure every PR is tested before being merged.

Describe alternatives you've considered
-

Additional context
-

@TheDanniCraft TheDanniCraft added bug Something isn't working enhancement New feature or request labels Oct 15, 2024
@TheDanniCraft TheDanniCraft self-assigned this Oct 15, 2024
@TheDanniCraft TheDanniCraft added this to the v1 Release milestone Oct 15, 2024
@TheDanniCraft TheDanniCraft added help wanted Extra attention is needed and removed bug Something isn't working labels Oct 15, 2024
@Muhammad-Owais-Warsi
Copy link

Hey @TheDanniCraft,
I would love to contribute to this!

I have a question: Should the tests run before the user raises the PR, or should they run on GitHub itself?
I’ll be needing your assistance as this is my first time working on an issue like this.

Thanks!

@TheDanniCraft
Copy link
Owner Author

Hey @Muhammad-Owais-Warsi, 👋
Thanks so much for your interest in contributing to this issue! ❤️

The tests should ideally serve two purposes:

  1. Self-testing before creating a PR: Running the tests locally helps ensure changes work as expected and don’t accidentally break anything.
  2. Testing PRs on GitHub: The tests will also run automatically on GitHub as part of the CI workflow to verify everything in a consistent environment.

My plan is to aim for tests with good code coverage, ideally including mocked functions and API responses where needed, to make them reliable and efficient. That said, feel free to focus on what you’re comfortable with or what you can manage—every contribution helps!

Let me know if you have any questions or need guidance. I’m here to help! Looking forward to your contributions. 🚀

@Muhammad-Owais-Warsi
Copy link

Thanks a lot @TheDanniCraft . Actually I don't even know ehre to start from, I have gone through the jest documentation, but how do i test the functions inside the github.js file which need dynamic data.

@TheDanniCraft
Copy link
Owner Author

Thanks a lot @TheDanniCraft . Actually I don't even know ehre to start from, I have gone through the jest documentation, but how do i test the functions inside the github.js file which need dynamic data.

No worries, I’m not super familiar with jest either, but I think what we’ll need to do is mock the API responses to handle the dynamic data. This way, we can simulate how the functions behave without actually hitting the real API (Jest Mock Functions).

Feel free to give it a try, and let me know if you run into any issues—I’ll try my best to help out! 😊

@adeson33
Copy link

adeson33 commented Jan 9, 2025

hey @TheDanniCraft
i have see your question in opire, i am a c++ engineer not familiar in jest framework. it sounds like you want build a CD system to run the unit test.If you need help , I can join you.
And by the way, I have a question in using the activity-log, why my operation in github readme log were always repeated,it's my generated log.
Snipaste_2025-01-09_09-10-52

@adeson33
Copy link

adeson33 commented Jan 9, 2025

hey @TheDanniCraft
I don't know which is the function point we have to test, or now the target is to build a automative test env. If we want to work together efficiently, i think you can provide us a list of test function points, and each of us can do a part.

@TheDanniCraft
Copy link
Owner Author

Hey @adeson33,

Thank you so much for your interest in helping out, I really appreciate it! ❤️

Regarding your issue with the activity-log: could you please create a separate issue for that? This will help us keep discussions focused and address your concern about the repeated logs more effectively.

As for the testing setup, the main goals are:

  • Build an automated test environment to reliably find breaking changes (e.g., block merge of prs if they break the action).
  • Allow contributors to self-test their code locally before submitting PRs to catch issues early.

I agree that having a clear list of test function points would make the process more efficient. However, I’ll admit I don’t have much experience with test coverage strategies, so I’m not sure what functions would make the most sense to test. If you or anyone else has suggestions for a good testing strategy, I’d love to hear them!

Let me know if you’d like to take on any part of this process or if you have ideas for how we should proceed—I’m happy to collaborate. 😊

@adeson33
Copy link

adeson33 commented Jan 9, 2025

@TheDanniCraft
Of course i'd like to join you, i will create a seperate issue later. Oh, you don't have to be nervous about the test coverage strategies. In my work experience, you only have to make sure primary function point not wrong. And when other users or our team members discover the bug, we can add the operation to complement our unit test.

@Muhammad-Owais-Warsi
Copy link

That's great. I think the main functions are in the github.js file that fetches all the data and performs the operation. As @adeson33 suggested we can seperate the part for each of us and collaborate efficiently.

@TheDanniCraft
Copy link
Owner Author

@adeson33 @Muhammad-Owais-Warsi Thanks for giving it a try! ❤️

I found this article on dev.to, maybe it’ll help you in writing the unit tests.

Also, I think it might be a good idea if you make a checklist of the tests you’re working on. This way, we avoid duplicate efforts and ensure everything is covered without overlap. Once a task is completed, mark it off to keep track of the progress efficiently.

@Muhammad-Owais-Warsi
Copy link

I was going through the codebase and jest documentation only and I think I can give try to the functions in github.js file first.

@adeson33
Copy link

adeson33 commented Jan 10, 2025

hey @Muhammad-Owais-Warsi , nice to work with you
i don't know what you mean the github.js file, i don't see the source code in the database of the activity-log,can you point out the path or show me the screenshoot.
src_files

@TheDanniCraft
Copy link
Owner Author

@adeson33 you can find the github.js file under src/utils/github.js

@Muhammad-Owais-Warsi
Copy link

Hey, @TheDanniCraft @adeson33 , I've tried of writing a test for this function in the src/utils/githib.js file. Wrote it inside the gist for now.

Once verified we can create a folder and start writing them in the actual codebase. Thanks:)

https://github.com/TheDanniCraft/activity-log/blob/master/src/utils/github.js#L10

@TheDanniCraft
Copy link
Owner Author

TheDanniCraft commented Jan 10, 2025

Hey @Muhammad-Owais-Warsi,

Thanks for sharing your test! You’re definitely on the right track, but I noticed a few issues (Copilot helped to catch some of them) that might need addressing to make the test work properly:

  1. Mock the octokit.rest.activity.listReposStarredByAuthenticatedUser Method:
    Instead of relying on the actual API, we should mock the listReposStarredByAuthenticatedUser function. This allows us to simulate API responses and control the test environment without needing a live GitHub connection. With this approach, you also won’t need the GITHUB_TOKEN.

  2. Missing octokit Parameter in fetchAllStarredRepos:
    The fetchAllStarredRepos function doesn’t currently accept octokit as a parameter, which makes the test non-functional.

  3. Improper Assertion on Set: The line expect(result.starredRepoNames).toBeInstanceOf(Set) won’t work because Jest cannot directly validate a Set type. Instead, you can convert it to an array and compare its values using Array.from(result.starredRepoNames).

  4. Pagination Not Tested Thoroughly:
    The function supports paginated responses, so the test should verify that it handles multiple pages of results correctly.

  5. Error Handling Not Tested:
    The function includes error-handling logic with core.setFailed. This should be tested to ensure it behaves as expected when API calls fail.

Here’s a Copilot-generated version of the test that incorporates these improvements (feel free to tweak as needed). @adeson33, maybe you can review this to verify if these tests are good:

import { getOctokit } from '@actions/github';
import { fetchAllStarredRepos } from './path-to-func';

jest.mock('@actions/github', () => ({
    getOctokit: jest.fn(() => ({
        rest: {
            activity: {
                listReposStarredByAuthenticatedUser: jest.fn(),
            },
        },
    })),
}));

describe('FetchRepoAndFilterEvents', () => {
    afterEach(() => {
        jest.clearAllMocks();
    });

    it('should fetch all starred repositories and return a set of repo names', async () => {
        // Mock paginated responses
        getOctokit().rest.activity.listReposStarredByAuthenticatedUser
            .mockResolvedValueOnce({ data: [{ owner: { login: 'user1' }, name: 'repo1' }] })
            .mockResolvedValueOnce({ data: [] }); // End of pagination

        const result = await fetchAllStarredRepos();

        expect(result).toHaveProperty('starredRepoNames');
        expect(Array.from(result.starredRepoNames)).toEqual(['user1/repo1']);
    });

    it('should handle errors gracefully', async () => {
        // Simulate API error
        getOctokit().rest.activity.listReposStarredByAuthenticatedUser.mockRejectedValueOnce(new Error('API Error'));

        await expect(fetchAllStarredRepos()).rejects.toThrow('API Error');
    });
});

Lastly, you might want to consider opening a draft PR for your work. This way, everyone can collaborate directly on your progress and share feedback.
Let me know what you think or if you need any help! 😊

@Muhammad-Owais-Warsi
Copy link

Muhammad-Owais-Warsi commented Jan 10, 2025

Thanks a lot for pointing out the corrections. I'll make the corrections and will let you know.
My doubts are-

  1. If we are testing by mocking, is it necessary to call the actual API. Instead we directly call the API or just test it by mocking it.
const result = await fetchAllStarredRepos();
  1. The result can return different values, in that case it can fail
expect(Array.from(result.starredRepoNames)).toEqual(['user1/repo1']);

Thanks.

@TheDanniCraft
Copy link
Owner Author

The goal here is to mock the API responses instead of calling the real API. By mocking, we ensure that the input data is always the same, making the tests predictable and replicable. This way, we avoid issues with fluctuating API data or network problems, and we have full control over the responses we want to test.

For example, in the test, we mock the response like this:

octokit.rest.activity.listReposStarredByAuthenticatedUser
    .mockResolvedValueOnce({ data: [{ owner: { login: 'user1' }, name: 'repo1' }] })
    .mockResolvedValueOnce({ data: [] }); // End of pagination

This ensures that the test always uses the same data and can be reliably run every time, no matter what happens in the actual API.

Let me know if you need further clarification! 😊

@Muhammad-Owais-Warsi
Copy link

Thanks a lot for pointing out the corrections. I'll make the corrections and will let you know. My doubts are-

  1. If we are testing by mocking, is it necessary to call the actual API. Instead we directly call the API or just test it by mocking it.
const result = await fetchAllStarredRepos();
  1. The result can return different values, in that case it can fail
expect(Array.from(result.starredRepoNames)).toEqual(['user1/repo1']);

Thanks.

Thanks for the clarification,but I think you misunderstood my question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants