init-package-json icon indicating copy to clipboard operation
init-package-json copied to clipboard

fix: fix issue with retrieving repository URL

Open marshallku opened this issue 1 year ago • 8 comments

- gconf = gconf.split(/\r?\n/)
+ const gconf = await fs.readFile('.git/config', 'utf8').catch(() => '')
+ const lines = gconf.split(/\r?\n/)

In #186, there was breaking change in retrieving repository URL from .git/config.

Although the split lines were assigned to lines, the code still accessed gconf to get the file lines, causing the repository field to never auto-complete.

References

marshallku avatar Jun 26 '24 01:06 marshallku

We will definitely want a regression test on this one.

wraithgar avatar Jun 26 '24 14:06 wraithgar

Hi, @wraithgar
The logic is almost same as previous version(before breaking changes), and I just tested locally with my git config file.
Should I write test code for this case?

marshallku avatar Jun 26 '24 15:06 marshallku

@marshallku yes. The fact that tests didn't fail w/ this bug means the tests are lacking here. We need some test that would fail w/ the old code but succeed w/ the new code.

wraithgar avatar Jun 26 '24 15:06 wraithgar

It may be as simple as an assertion in an existing test that wasn't looking at the affected attributes. I haven't dug in to be sure.

wraithgar avatar Jun 26 '24 15:06 wraithgar

@wraithgar I add test case for retriving URL of remote repository. I used git config --get remote.origin.url in runtime to get the expected output. Let me know if it would be better to hard-code https://github.com/npm/init-package-json.git.

marshallku avatar Jun 26 '24 23:06 marshallku

We don't need to hard code it, but the test should probably be doing the same thing that the code itself is doing, namely looking in the git config file to find the right value.

We'll have to hope that file is present in CI. We've had tests in the past that were coupled to the actual .git content of the source, which failed in some test setups. If that happens we'll have to generate a new test fixture directory for this test and manually write that file.

ETA: Yep, tests fail in CI cause the remote is subtly different than you have locally. We'll want to use a tap testdir here so we can control what goes into that file.

wraithgar avatar Jun 27 '24 14:06 wraithgar

Thanks for leading @wraithgar. I used testdir, and add more cases to verify logics.

marshallku avatar Jun 28 '24 01:06 marshallku

Hi @wraithgar

I wanted to kindly remind you about the pull request I submitted. Your feedback is highly valuable, and I’d appreciate it if you could take some time to review it.

marshallku avatar Jul 16 '24 01:07 marshallku

superceded by #315

wraithgar avatar Dec 02 '24 16:12 wraithgar