[BUG] Test failure related to terminal environment about pueue HOT 17 CLOSED

I'm seeing some new failures when I pull in this commit along with the other commits since v2.1.0. You've done more than enough already. I will do some investigation tomorrow to see if I can find out more.

from pueue.

I now think maybe a way to fix this is to approach it by creating the expected output not as a Handlebars template but by building it using comfy-table directly. This might make the test logic itself too complicated to be palatable.

I had the same idea, but this kind of misses the point. We would basically test two identical pieces of logic against each other.
The idea behind those template tests was to have a manual check of the expected output. With this approach, any changes in the rendered output will be detected. This includes changes in libraries such as comfy-table.

Please feel free to close this issue if you think so. I'm sorry I led you down this convoluted path.

No worries :D I didn't expect the client tests to cause this many problems in different environments^^. It was an interesting bug hunt!

Besides, we managed to get the daemon tests up and running, which tests pretty much all of the critical internal and API logic.

from pueue.

Hmmm. This is a bit tricky.

Comfy-table looks at the current TTY and checks its width to determine how to fit the content into the table.
I guess we could force comfy-table to always use a non-dynamic table width, if a certian environment variable is detected?

We could then inject that environment variable into the client's env when running the tests. Normal user's wouldn't need to know about it.

I'll have a look at it soon :)

from pueue.

Can you test whether your test run works with the unflake-tests branch?

from pueue.

I'm sorry I accidentally misled you about the terminal width. In theory it can be a factor, but I don't think it's leading to these test failures. After digging into it, there are two causes that lead to almost all of these failures.

1. In the nixpkgs build, the TMPDIR environment variable is set to /build instead of /tmp, which leads to a mismatch in the number of spaces after the path in table cells that contain them. This can be set up in the tests, or I can also treat it as a pre-condition on the environment and configure it in the nixpkgs build itself.

2. When a task is spawned by the server, it clears the environment before copying over environment variables specified in the AddMessage. In tests, the AddMessage does not contain a PATH, and so the spawned process only has default paths that are hardcoded (maybe by glibc). In nixpkgs, however, the build environment is sandboxed from the system so basic utilities like sleep cannot be found.

from pueue.

1. This one is tricky. I don't really see a way around using /tmp if we plan to use the current system to compare output.

   One thing we could do is to use different templates depending on the environment.
   However, this doesn't seem very feasible, as it would then be pretty confusing and cumbersome for devs to work with templates. They would have to always create/update two templates for each adjustment they make and they would have to run all client tests twice with different settings to simulate the different environments.

   So, if this wouldn't be too much of a problem, having TMPDIR point to /tmp would be pretty nice.
   Otherwise, I'll try to come up with a solution for this problem.

2. This should work automatically for client tests, as pueue catches the current environment and injects it into the task.
   The daemon tests however don't do this.
   I guess we could probably catch the current environment whenever we create raw AddMessage structs?

   This should be fairly straight forward.

NixOs Ticket for reference: NixOS/nixpkgs#185747

from pueue.

The linked MR should fix the second source of failures :)

branch: catch-env-in-daemon-tests

from pueue.

When I apply this patch and set TMPDIR locally, all tests pass except for daemon::edit::test_edit_flow, which can be looked at separately.

I am now worried about setting TMPDIR in the nixpkgs build because it could interfere with other builds that also depend on this environment variable. Would it be possible to use TempDir#new_in to force it to /tmp in the tests themselves?

from pueue.

Nice. I'll merge that MR first and I'll think about a solution to hack around the table formatting problem :)

from pueue.

The idea behind these tests is to have a visual check that the expected output looks the same as the actual output.

The expected output is defined by some templates, which is pretty handy.
They look like this:

Group "default" (1 parallel): running
────────────────────────────────────────────────────────────────
 Id   Status    Command   Path              Start      End      
════════════════════════════════════════════════════════════════
 0    Success   ls        {{ cwd }}   {{ task_0_start }}   {{ task_0_end }} 
────────────────────────────────────────────────────────────────

However, when working with formatted tables, the lenght of the table depends on the table's content.
The difference in characters of /tmp and /build results in the table being 2 chars longer in your case than in a "normal" environment.

I'll take a look, if we can somehow template the length of the table

from pueue.

@tjni I have an idea.

Do you have read permissions to /tmp or / in your build/test environment?
If so, we could just set the cwd of these tasks to either of those.
These tasks wouldn't write or read in these directories, they just use them as cwd.

These directories should be a common denominator on all unix systems.
This could prevent us from having to work with weird templating hacks.

I created a POC MR which changes the cwd of all tasks in our client tests to /tmp.

Could you take a look if the new commit on #343 fixes your issues?

from pueue.

Thank you for the proposal and proof-of-concept. I haven't tried it yet because I started reading more about the sandbox that packages can be built with in Nix. It seems that sandboxing is enabled on the build infrastructure that nixpkgs uses. When sandboxed, /tmp is not accessible by the build process.

This could be a situation where it's not worth the effort to support this non-standard setup in the test. In nixpkgs, the client tests are currently disabled to work around this problem, and we can continue to do so.

To support, I guess it would be possible to create some sort of context variable representing "length of path" prefix and use that in all of the other cells of that column in the table.

from pueue.

I wasn't able to get to this tonight (in my timezone), but I want to offer to take a stab at the idea I gave in my last comment tomorrow, with a placeholder for the length of the path. I feel it's not worth much more of your time on this unless it really piques your interest. You can take a look at the attempt and determine if it's too complicated for the project or not.

from pueue.

Oh yuck, I started trying to do this and realize it's impossible.

I now think maybe a way to fix this is to approach it by creating the expected output not as a Handlebars template but by building it using comfy-table directly. This might make the test logic itself too complicated to be palatable.

Please feel free to close this issue if you think so. I'm sorry I led you down this convoluted path.

from pueue.

I'm going to close this issue for now, as I'm out of ideas as well.

Feel free to re-open if you have another idea on how to solve this problem! I'll do the same!

Thanks again for the report and the quick response times!

Cheers

from pueue.

This will most likely be addressed by #390

The same test issues appeared on the apple platform and I'm planning to restructure the client testing approach a bit, to prevent such things in the future :)

from pueue.

I look forward to it. Thank you!

from pueue.