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

fix a couple of clippy findings and run subcrate tests on travis #1468

Merged
merged 2 commits into from
Jul 25, 2018

Conversation

matthiaskrgr
Copy link
Contributor

No description provided.

@chrisduerr
Copy link
Member

Thanks for your contribution! I'd assume that you've found the clippy warnings by running cargo clippy inside of the font subcrate, but how did you find the issue in the main.rs? Clippy doesn't seem to lint that for me.

Also looking at this it might make sense to handle the testing of the font subcrate differently. Currently none of the tests inside of font are run when something inside of font is changed and clippy doesn't run on the code change either. Would be interesting to see if Github/Travis-CI support subdirectories more properly for CI purposes.

@matthiaskrgr
Copy link
Contributor Author

Right, I was playing around with some additional warnings, the main.rs one is a clippy needless_borrow which is silent be default.

@chrisduerr
Copy link
Member

Oh it seems like there's a false positive currently present in the needless_borrow lint in combination with derive macros, which is why it's temporarily disabled. That explains why clippy did not warn about this anymore.

@chrisduerr
Copy link
Member

Also it seems like clippy does properly lint the subcrates (even though I wasn't able to reproduce locally).

See https://travis-ci.org/jwilm/alacritty/jobs/408024826.

@matthiaskrgr
Copy link
Contributor Author

I think it might only lint the subcrates due to --all-targets --all-features https://github.com/jwilm/alacritty/blob/master/.travis.yml#L32

I'll remove the needless_borrow change from the pr.

@matthiaskrgr
Copy link
Contributor Author

I added another commit which runs the tests of the two subcrates on travis as well.

It seems to me that cargo test --all should actually achieve this but as well but I couldn't see any of the subcrate tests being run so I called the tests one by one.

@chrisduerr
Copy link
Member

I don't think removing the needless_borrow fix is necessary. It's just that this is the reason why it's currently disabled. When there are no false positives, the lint should still be followed.

@matthiaskrgr matthiaskrgr force-pushed the master_warnings branch 2 times, most recently from cdf0ddc to 0fa5e50 Compare July 25, 2018 15:27
change  println!("");  to  println!();
fix needless_borrow clippy lint finding.
@matthiaskrgr
Copy link
Contributor Author

matthiaskrgr commented Jul 25, 2018

Ok I think the copypasta tests fail because travis is headless and thus there is no x11 clipboard. :/

test x11::tests::clipboard_works ... FAILED
failures:
---- x11::tests::clipboard_works stdout ----
	thread 'x11::tests::clipboard_works' panicked at 'store selection: Io(Os { code: 2, kind: NotFound, message: "No such file or directory" })', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    x11::tests::clipboard_works

I'll undo running this test on travis again...

@jedahan
Copy link
Contributor

jedahan commented Jul 25, 2018

@matthiaskrgr
Copy link
Contributor Author

@jedahan ah, that's a great suggestion, thanks, I'll try that! :)

@matthiaskrgr matthiaskrgr changed the title fix a couple of clippy findings. fix a couple of clippy findings and run subcrate tests on travis Jul 25, 2018
@matthiaskrgr matthiaskrgr force-pushed the master_warnings branch 2 times, most recently from 01cc4b8 to c36affd Compare July 25, 2018 16:33
@chrisduerr
Copy link
Member

I don't think using xvfb to run tests would be a great plan. I've used it for benchmarking before, so I have a travis config which would make this work, however it seems quite unclean to me.

Keeping in mind that the copypasta crate is going to be replaced long-term anyways, I'd be fine with skipping the test for CI for that. Since it's unlikely that this subcrate is going to change.

testing of copypasta crate was omitted due to problens when running on headless travis-ci environment
(x11 clipboard would fail)
@matthiaskrgr
Copy link
Contributor Author

Ok I re-removed copypasta testing on travis ci.

@chrisduerr chrisduerr merged commit ddb9a55 into alacritty:master Jul 25, 2018
@matthiaskrgr matthiaskrgr deleted the master_warnings branch July 27, 2018 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants