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.

Sign up for GitHub

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

Jump to bottom

Ensure test suite and file rotation work on Windows #509

Merged
merged 36 commits into from
Aug 10, 2019
Merged

Ensure test suite and file rotation work on Windows #509

merged 36 commits into from
Aug 10, 2019

Conversation

lukebakken
Copy link
Contributor

Follow-up to #493

lukebakken added 13 commits July 29, 2019 15:01
@lukebakken
Remove code causing some warnings
f03fa28
@lukebakken
Fix rotate_file_fail_test on win32
a4ceb21
@lukebakken
Making progress on getting lager_file_backend tests passing on win32
990210f
@lukebakken
All but one lager_file_backend test passes on win32
3267b98
@lukebakken
Fix a couple more win32 test issues in lager_file_backend
ecb0f8c
@lukebakken
Fix "Internal rotation and delayed write" test on win32
39c1248
@lukebakken
Refactor lager_rotator_behaviour to return a files ctime value, which…
7bb0988 
… is necessary for detecting changes on win32
@lukebakken
Take a file ctime into account when write/4 is called
439e82c
@lukebakken
Fix test in lager_file_backend with exception for win32
c7d7fce
@lukebakken
Ensure creation time is updated on win32 when files are created
0bc318c
@lukebakken
Write test files using 1 second creation time in the future, lager_fi…
3e9e4eb 
…le_backend suite passes on win32
@lukebakken
Add inode check to write_should_check
62e847a
@lukebakken
Factor out common code into lager_util:has_file_changed
cd5f69d
@lukebakken lukebakken mentioned this pull request Aug 2, 2019
Closed
@jadeallenx
Copy link
Member

Thanks for taking this on

lukebakken added 15 commits August 2, 2019 09:56
@lukebakken lukebakken mentioned this pull request Aug 5, 2019
Merged
@lukebakken lukebakken marked this pull request as ready for review August 5, 2019 21:23
@lukebakken
Copy link
Contributor Author

Hi @mrallen1!

I have confirmed today that these changes do address the file rotation issues in RabbitMQ on win32. I'm continuing to test but I figure getting a review started would be nice.

There is a breaking API change in lager_rotator_behaviour in that a file's creation time is passed as an argument. I suppose we could get clever in working around that breaking change if you'd like, or use this as an opportunity to release Lager 4.0.0.

What do you think?

@jadeallenx
Copy link
Member

Looks like solid work but tbh I'm not a huge fan of so many whitespace changes - all the noise makes it hard to trace what "real" changes have been made. As far as breaking the lager_rotator_behaviour I think we can tag a new release as 3.8.0 with appropriate warnings in the README.

It would be great to get a blurb in the README as part of this PR.

@lukebakken
Copy link
Contributor Author

lukebakken commented Aug 6, 2019 •
edited
Loading

@mrallen1 there was one file formatted with 2-space indentation (test/lager_test_function_transform.erl). Can you give the "ignore whitespace" review link a try before I revert that?

https://github.com/erlang-lager/lager/pull/509/files?w=1

I'll add a README update today. Still testing!

lukebakken
lukebakken commented Aug 6, 2019
View reviewed changes
test/pr_stacktrace_test.erl
throw:{test,exception}",
?assertNotEqual(0, string:str(Result, ExpectedPart)).

Want = "pr_stacktrace_test:pr_stacktrace_throw_test/0 line 26\n pr_stacktrace_test:make_throw/0 line 16\nthrow:{test,exception}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I clone this repo on win32, my git settings check out files with \r\n endings, which makes this test fail due to embedded \r characters in the long string. This is why these expected strings are on a single line now.

@lukebakken lukebakken mentioned this pull request Aug 6, 2019
Merged
@lukebakken
Copy link
Contributor Author

Well, it looks like even the master branch has a heisen-bug in the test suite, but only on Travis CI. Everything runs fine on my OS X laptop, Linux and Windows workstations. I'll continue to investigate.

@michaelklishin
Copy link

Yay, Travis is green 💚

@jadeallenx jadeallenx merged commit 6825530 into erlang-lager:master Aug 10, 2019
@jadeallenx
Copy link
Member

Thanks again for getting win32 on a good level, Luke. Your contributions are much appreciated!

@jadeallenx
Copy link
Member

Incidentally, I tagged and pushed 3.8.0 to hex just now.

@lukebakken lukebakken deleted the lrb-windows-file-rotation branch August 11, 2019 18:00
@lukebakken
Copy link
Contributor Author

Thanks @mrallen1 👍

@lukebakken
Copy link
Contributor Author

@mrallen1 if you get a chance, could you enable the lager build on AppVeyor?

https://ci.appveyor.com/project/erlang-lager

@jadeallenx
Copy link
Member

It says "not found" for some reason

@jadeallenx
Copy link
Member

OK, seems to be going now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lukebakken @jadeallenx @michaelklishin