Skip to content

Conversation

@gowthamkishore3799
Copy link
Contributor

Summary:

  1. Added test cases for CLI with default options.
  2. Added test cases for CLI with custom options passed from the command line.

Implementation Notes:

  1. Ensured the default CLI options create the expected output without additional arguments.
  2. Validated custom CLI options (e.g., custom file paths, max size, and exclude patterns) also work seamlessly.

Alternative Options Considered:
Instead of actually creating and removing the output file during the test, I considered mocking the os.open and related functions. However, I opted to test the entire process end-to-end to ensure seamless functionality.

@gowthamkishore3799 gowthamkishore3799 marked this pull request as ready for review January 19, 2025 23:54
Copy link
Member

@cyclotruc cyclotruc left a comment

Choose a reason for hiding this comment

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

Thank you very much for your work!

image
I found those tests quite slow to run when i tried them in a codespace (4 cores 8gb ram)
Being slow to run means they risk being less helpful in a TDD context and I don't think that's something we want from the main test suite

So we can see it one of two ways:
1 - They are end-to-end tests and should be separated from the main test suite
2 - They are unit test so they should mock the necessary components to only test units and not integration

Happy to discuss this further if it's unclear what I mean by that

@cyclotruc cyclotruc added the changes requested A review requested changes before merging label Jan 21, 2025
@gowthamkishore3799
Copy link
Contributor Author

@cyclotruc Made a change to path which reduced the execution time right now, Initally had the path outside of directory, which should have been "./"

Screenshot 2025-01-20 at 7 42 08 PM

Copy link
Member

@cyclotruc cyclotruc left a comment

Choose a reason for hiding this comment

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

Thanks a lot!
I tested it by breaking the CLi on purpose and the test failed, which is all we really needed

@cyclotruc cyclotruc merged commit 1836809 into coderamp-labs:main Jan 22, 2025
8 checks passed
prechayimmee2412 added a commit to prechayimmee2412/gitingest that referenced this pull request Jan 22, 2025
Co-authored-by: Gowtham Kishore <70622086+gowthamkishore3799@users.noreply.github.com>
FOLKS-Tech pushed a commit to FOLKS-Tech/gitingest that referenced this pull request Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested A review requested changes before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants