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

DEFAULT_VIEWPORT_WIDTH too narrow for non tty gitlab runners #5919

Closed
majnos opened this issue Feb 5, 2021 · 6 comments · Fixed by #6930
Closed

DEFAULT_VIEWPORT_WIDTH too narrow for non tty gitlab runners #5919

majnos opened this issue Feb 5, 2021 · 6 comments · Fixed by #6930
Labels
TYPE: enhancement The accepted proposal for future implementation.

Comments

@majnos
Copy link

majnos commented Feb 5, 2021

What is your Test Scenario?

The problem occurs when testcafe is run by non-interactive runner. E.g. gitlab docker-machine runner. By default this runner runs its jobs in non-interactive mode(in other words non-tty). That is why this line fails:

get-viewport-width.js:
if (outStream === process.stdout && tty.isatty(1)) {

And DEFAULT_VIEWPORT_WIDTH is set. As a result all the formatted lines are cut to 78 chars. Which is very limiting. In my case this constant limits length of SELECTOR shown in exceptions to 1/2 of full length. That makes my debugging more complicated. I am suggesting an override to this constant so I am able to see whole selector.

What are you suggesting?

I am suggesting to be able to override this default value by env variable. It could be also placed in some env file if there is some?

What alternatives have you considered?

For non-tty runners there is no alternative then to switch to tty-runners. That means register gitlab runner (docker-executor) as interactive.

Additional context

Whole proposed change:

get-viewport-width.js:

import tty from 'tty';

const DEFAULT_VIEWPORT_WIDTH = process.env.DEFAULT_VIEWPORT_WIDTH || 78;

export default function (outStream) {
    if (outStream === process.stdout && tty.isatty(1)) {
        const detectedViewportWidth = process.stdout.getWindowSize ?
            process.stdout.getWindowSize(1)[0] :
            tty.getWindowSize()[1];

        return Math.max(detectedViewportWidth, DEFAULT_VIEWPORT_WIDTH);
    }

    return DEFAULT_VIEWPORT_WIDTH;
}
@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Feb 5, 2021
@felis2803
Copy link
Contributor

Thank you for your suggestion. We are not sure if environment variables should be used for this purpose. Please give us additional time to research this.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Feb 8, 2021
@miherlosev miherlosev added the STATE: Need improvement A minor issue that can't be treated as a bug. label Feb 10, 2021
@AndreyBelym AndreyBelym added TYPE: enhancement The accepted proposal for future implementation. and removed STATE: Need improvement A minor issue that can't be treated as a bug. labels Jun 7, 2021
@PayBas
Copy link
Contributor

PayBas commented Mar 17, 2022

Yes please! The output from our Jenkins CI server jobs is miserably narrow, which makes automated failure analysis very difficult.

     38 |        await t.scrollIntoView(selector);
     39 |
   > 40 |        await t.expect(selector.innerText).contains(status, 'el
  has the right status', { timeout: 60_000 });
     41 |    }
     42 |}
     43 |

As far as I know, it is impossible to start a TTY with a set width/columns. Just a quick Google search shows that trying to control this in an OS-agnostic way is a fools errand.

So having some way to override this DEFAULT_VIEWPORT_WIDTH value for CI servers would be a life-saver. Although I'd name it TC_DEFAULT_VIEWPORT_WIDTH to prevent any confusion as to what it's for.

If needed, I'll make the PR myself.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Mar 17, 2022
@VasilyStrelyaev
Copy link
Collaborator

Thank you for your suggestion. We would greatly appreciate your help. You are welcome to open a PR.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Mar 17, 2022
@PayBas
Copy link
Contributor

PayBas commented Mar 18, 2022

You are welcome to open a PR.

@VasilyStrelyaev Done

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Mar 18, 2022
@VasilyStrelyaev
Copy link
Collaborator

Thank you, @PayBas, we will take a look!

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Mar 18, 2022
@github-actions
Copy link

Release v1.20.0-alpha.1 addresses this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TYPE: enhancement The accepted proposal for future implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants