Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

NGit.Api.ApplyCommandTest tests failing on Windows #36

Open
dprothero opened this issue Jul 7, 2012 · 19 comments
Open

NGit.Api.ApplyCommandTest tests failing on Windows #36

dprothero opened this issue Jul 7, 2012 · 19 comments

Comments

@dprothero
Copy link

I have another set of tests failing on Windows.

NGit.Api.ApplyCommandTest.TestAddA1:
NGit.Errors.InvalidObjectIdException : Invalid id : de98044

NGit.Api.ApplyCommandTest.TestAddA2:
NGit.Errors.InvalidObjectIdException : Invalid id : de98044

NGit.Api.ApplyCommandTest.TestDeleteD:
NGit.Errors.InvalidObjectIdException : Invalid id : 0000000

Those are throwing an IndexOutOfRangeException in RawParseUtils.ParseHextInt32, which FromHexString catches and rethrows as an InvalidObjectIdException

Then, the remaining tests in NGit.Api.ApplyCommandTest are failing with a different error:

NGit.Api.ApplyCommandTest.TestModifyE:
System.ArgumentException : Illegal characters in path.

NGit.Api.ApplyCommandTest.TestModifyX:
System.ArgumentException : Illegal characters in path.

NGit.Api.ApplyCommandTest.TestModifyY:
System.ArgumentException : Illegal characters in path.

NGit.Api.ApplyCommandTest.TestModifyY:
System.ArgumentException : Illegal characters in path.

These are getting kicked out of the FilePath constructor from this line: this.path = Path.Combine (other, child);

For TestModityE, for example, other == "C:\Users\dprothero\Documents\GitHub\ngit\bin\target\trash\test1341691177321_542" and child == "E\r"

I'm assuming it doesn't like the backslash?

I'm still way too new to the codebase to be able to see what is going on with these issues.

@dprothero
Copy link
Author

There's a number of other failing tests. I put the XML results into my fork:

https://github.com/dprothero/ngit/blob/master/TestResult.xml

Do you want me to open other issues for each different type of failure, or use this issue (or perhaps Issue #33) for all of them?

@linquize
Copy link
Contributor

linquize commented Jul 9, 2012

I think the problem is related to FilePath class.
Issue #35 is about the rewrite of Sharpen.FilePath constructor. Are you interested in contributing to the rewrite to match most behaviours of java.io.File class?

@WilbertOnGithub
Copy link

Wondering if I need to open a separate issue for this, but it seems related to this question.

When doing a clean compile and running all unittests (on plain old Windows), I have quite a lot of failing tests. I have 39 failing tests (out of 2216).

Alt text

Is this a known issue?

@alanmcgovern
Copy link
Member

The NGit.Apply tests were failing because about 2 dozen new test files were added and autocrlf was not disabled. The test assumes linux file endings but git was changing them to crlf on checkout. This is fixed now. I'll investigate the rest of the issues after I finish lunch.

@linquize
Copy link
Contributor

linquize commented Jul 9, 2012

Is it due to current jgit cannot handle file larger than 8kb to convert autocrlf ?
someone has tried to fix that but later reverted. jgit spearce said we should not abuse (enlarge ) that 8k buffer as a workaround in 18 Mar 2012

@alanmcgovern
Copy link
Member

This particular issue was just files being incorrectly stored in ngits github repository. I'm not aware of an 8kb limitation on file sizes when trying to run autocrlf, but that seems ridiculously small. Most source code files will be dozens (or hundreds) of kb in size. Is 8kb really a hard limit for performing autocrlf? Where is this in the jgit/ngit source?

@linquize
Copy link
Contributor

linquize commented Jul 9, 2012

current approach to handle autocrlf is incorrect. it should be able to handle any size of file <2G but without requiring large memory buffer

@alanmcgovern
Copy link
Member

It sounds like it's a jgit limitation so i'd much rather it were fixed upstream than here. It sounds like they know about the issue so the best we can do is comment on their bug report and get it fixed. It did seem like proper support for this was added a few months ago as there is an AutoCRLFStream now which seems to do the conversion on a block by block basis.

Either way, the unit tests are all green on windows now with the latest commits.

@dprothero
Copy link
Author

I've got one failing test (in addition to other failing tests from issue #33) still:

NGit.Treewalk.FileTreeIteratorTest.TestIsModifiedFileSmudged:
Expected: SMUDGED
But was: DIFFER_BY_TIMESTAMP

at NGit.Treewalk.FileTreeIteratorTest.TestIsModifiedFileSmudged() in C:\Users\dprothero\Documents\GitHub\ngit\NGit.Test\NGit.Treewalk\FileTreeIteratorTest.cs:line 227

@WilbertOnGithub
Copy link

I can confirm the failing test described above - I have the same issue. Wanted to write a comment, but dprothero was a bit quicker ;-)

BTW, keep up the good work on NGit!

@alanmcgovern alanmcgovern reopened this Jul 9, 2012
@alanmcgovern
Copy link
Member

For those of you with failing tests, how are you running the tests?

@alanmcgovern
Copy link
Member

Also, what version of windows

@dprothero
Copy link
Author

I'm running the tests within the NUnit GUI. On Windows 7 x64.

@alanmcgovern
Copy link
Member

I'm not sure what could cause that failure. The best option would be to try and capture the failure in the debugger and see what has gone wrong. Everything is definitely green for me on the latest commit when run using the NUnit GUI runner on 64bit Windows 7, so unless it starts happening to me locally there's not much I can do to diagnose this. It could be that I cannot reproduce it because IO is comparatively slow for me as I'm running in a VM.

@WilbertOnGithub
Copy link

I'm running on Windows 7, 64 bit as well using the NUnit GUI. One difference for me is that I'm logged in default as a limited user (non-admin).

I'll try to get some more information from the debugger.

@dprothero
Copy link
Author

Running as non-admin as well.

@dprothero
Copy link
Author

I don't see how this test could ever pass... it seems to be trying to make a change to a file, but hoping to keep the same timestamp? When I run it, it's this code:

if (fileLastModified != cacheLastModified)
{
    return WorkingTreeIterator.MetadataDiff.DIFFER_BY_TIMESTAMP;
}

In WorkingTreeIterator.cs (~ line 939) that is causing CompareMetadata() to return DIFFER_BY_TIMESTAMP. Only if the timestamps are identical is there a possibility of it returning SMUDGED.

So, in the test code, it's hoping no time passes between the two calls to WriteTrashFile():

WriteTrashFile("file", "content");
git.Add().AddFilepattern("file").Call();
WriteTrashFile("file", "conten2");

That doesn't even seem possible. Even a couple ticks have to go by for this code to run. Could different systems have different resolutions on the filesystem timer?

UPDATE: Just to clarify, I've stepped through with the debugger and fileLastModified is always different than cacheLastModified.

David

@linquize
Copy link
Contributor

NGit.Treewalk.FileTreeIteratorTest succeed in WinXP, but fail in Win 7 (32 and 64-bit)

@alanmcgovern
Copy link
Member

All of the failing tests appear to be race conditions which I cannot reproduce on my SSD. The fix for these issues should be upstreamed to jgit. If anyone can diagnose the race and issue a pull request, i'll try to upstream it so it's fixed for everyone. There's not much else I can do as I can't trigger the failures on my machine.

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

No branches or pull requests

4 participants