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

Consider to clarify the test messages which cargo test creates, so it's obvious this was done by a test. #371

Closed
netzego opened this issue Dec 15, 2021 · 14 comments

Comments

@netzego
Copy link

netzego commented Dec 15, 2021

Today i installed this repo with archlinux. The aur packages compiles the code and run the tests. I didn't pay much attention, and later i found the 2 notifications created by the test cargo test --no-default-features --features=tls-native,desktop-notifications --release. It took me 5 min to realize what actually happend. So i could remove my tinfoil hat :). And thx for this repo.

@trevarj
Copy link
Contributor

trevarj commented Dec 15, 2021

Funny!

Not sure if there is a way to disable this since the test notifications are coming from the tiny crate, but the notification code is in the libtiny_tui crate. So if we try to do conditional compilation (#cfg(test)) and use the dummy notify message, that won't help...

Maybe just changing the messages to be more descriptive might work.

@osa1
Copy link
Owner

osa1 commented Dec 15, 2021

Thanks for reporting. I also had a good laugh at this :-)

I agree that just improving the messages should be good enough.

@netzego
Copy link
Author

netzego commented Dec 15, 2021

:)

Maybe just changing the messages to be more descriptive might work.

Also agree on changing the message to somewhat descriptive should be fine. Also that the aur package runs the test is alright, imho. Based on the msg shown by notify, an user should figure out, what and why the notfication was triggered.

@trevarj
Copy link
Contributor

trevarj commented Dec 15, 2021

I guess the AUR package doesn't really need to run the test, right? Is that a standard thing in AUR packages?

@netzego
Copy link
Author

netzego commented Dec 15, 2021

I don't know. But aur packages are community packages. maybe, it's kind of an agreement or actually somewhere in the guidelines for packagers. I will drop a comment there as well.

@netzego
Copy link
Author

netzego commented Dec 15, 2021

I just checked the install scripts. At the moment there are three repo's for tiny. One of them is a binary. The others are compiled by the user -- as usual for aur packages. But just one of them, do the test. Which is done in the check() function. Here is the link to the PKGBUILD bash script:

@trevarj
Copy link
Contributor

trevarj commented Dec 15, 2021

It might make sense to submit a request to change the PKGBUILD file to use cargo check instead of cargo test.

check() {
  cd "${srcdir}/tiny-${pkgver}"
  -cargo test --no-default-features --features=tls-native,desktop-notifications --release
  +cargo check --no-default-features --features=tls-native,desktop-notifications --release
}

@netzego
Copy link
Author

netzego commented Dec 15, 2021

With this, the notifications won't pop up?

@trevarj
Copy link
Contributor

trevarj commented Dec 15, 2021

Right, because the tests will not run. The code and dependencies will be checked for errors, but now I'm not sure if it's actually better to just run through cargo build since it does full codegen... @osa1?

@netzego
Copy link
Author

netzego commented Dec 15, 2021

Okey, i have linked this issue at the comment section for that AUR package. So at least it's "documented" ;-)

@osa1
Copy link
Owner

osa1 commented Dec 15, 2021

Right, because the tests will not run. The code and dependencies will be checked for errors, but now I'm not sure if it's actually better to just run through cargo build since it does full codegen... @osa1?

My understanding is check() is supposed to run the tests. https://wiki.archlinux.org/title/creating_packages search for "check()", it says: (emphasis mine)

Place for calls to make check and similar testing routines. It is highly recommended to have check() as it helps to make sure software has been built correctly and works fine with its dependencies.

So check step should build and test.

@trevarj
Copy link
Contributor

trevarj commented Dec 16, 2021

My mistake then!

@netzego see above comment, you might want to redact your comment there, sorry 😞

@osa1
Copy link
Owner

osa1 commented Dec 16, 2021

I tried fixing this issue by disabling notifications for the servers in these test, but it turns out we privmsg tabs don't inherit notifier from their server tabs. I reported this as #372.

If we fix #372 first then this issue can be fixed by disabling the notifications in these tests.

@netzego
Copy link
Author

netzego commented Dec 16, 2021

@trevarj no problem. I've edit that comment.

If we fix #372 first then this issue can be fixed by disabling the notifications in these tests.

Actually your test is fine. Also form the AUR perspective. Just the text should really adjusted. My two cents. Besides, i really enjoyed tiny so far. It's simple and clean, and most of the shotcuts are appreciated. Thx!

@osa1 osa1 closed this as completed in 3be18d3 Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants