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

Easier debugging / test-specific configurations #216

Closed
jhildenbiddle opened this issue Jul 14, 2020 · 20 comments
Closed

Easier debugging / test-specific configurations #216

jhildenbiddle opened this issue Jul 14, 2020 · 20 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@jhildenbiddle
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I typically run all tests headless, but I often have failing tests that that require visually inspecting the page. Currently, this involves the following:

  1. Find my jest-playwright.config.js file
  2. Modify my browsers to include only one browser (since I generally don't need to visually inspect in multiple browsers)
  3. Modify my launchOptions to set headless:false and occasionally devtools:true

Once the test passes, I then have to undo all of the changes made above.

What would greatly speed up this process is a way to launch a single test in non-headless mode, possibly with devtools enabled, for one-or-more browsers and/or devices. Ideally, this would be as quick as adding .only or .skip to a test, although this would obviously take some pre-configuration.

Describe the solution you'd like

The verbose-but-flexible way would be to allow passing configuration options with each test. Using the existing jestPlaywright.skip implementation as inspiration, perhaps something like this could work:

jestPlaywright.config({ 
  browser: 'chromium',
  launchOptions: {
    headless: false,
    devtools: true,
  }
}, () => {
  // Tests...
})

This could be made less obnoxious by storing the non-headless/debug configuration separately and passing it to the .config() method:

const jestPlaywrightDebugConfig = {
  browser: 'chromium',
  launchOptions: {
    headless: false,
    devtools: true,
  }
};

jestPlaywright.config(jestPlaywrightDebugConfig, () => {
  // Tests...
})

Better yet, the non-headless/debug configuration could be set in jest-playwright.config.js, allowing a much simpler .debug() method to be used without passing additional parameters:

// jest-playwrite.config.js
module.exports = {
  browsers: [
    'chromium',
    'firefox',
    'webkit',
  ],
  debugOptions: {
    browsers: [
      'chromium'
    ],
    launchOptions: {
      headless: false,
      devtools: true
    }
  }
};
// my.test.js
jestPlaywright.debug(() => {
  // Tests...
})
@jhildenbiddle jhildenbiddle changed the title Ability to launch individual tests with headless:false Easier debugging / test-specific configurations Jul 14, 2020
@mmarkelov
Copy link
Member

@jhildenbiddle thank you for your suggestion. Seems like that it can be very helpful. I'll take it

@mmarkelov mmarkelov added the enhancement New feature or request label Jul 14, 2020
@mmarkelov mmarkelov self-assigned this Jul 14, 2020
@jhildenbiddle
Copy link
Contributor Author

Excellent! Thanks for considering the change, @mmarkelov.

@mmarkelov
Copy link
Member

@jhildenbiddle just look through this issue and made #233
You will be able to do something like that:

    test('test1', async () => {
        await jestPlaywright.config({
            browser: 'chromium',
            launchType: "LAUNCH",
            launchOptions: {
                headless: false,
                devtools: true,
            }
        }, async ({page}) => {
            // page is coming from function params
            await page.goto('https://github.com/')
            const title = await page.title()
            expect(title).toBe('Google')
        })
    })

So you can config playwright only inside the specific test. Personally I don't think that it's perfect decision, I just tried some another opportunities, like using jestPlaywright.config function inside describe, but cause it's sync. there is no ability to use it with async config function.

Also I don't know how we should config it, should we merge passing options with existing?

@mmarkelov
Copy link
Member

@jhildenbiddle also we just discussed some another ways to implement this feature with @mxschmitt
Maybe we can test it with CLI, for example:

// tests:
describe('Simple tests',  () => {
    beforeAll(async () => {
        await page.goto('https://google.com/')
    })

    test('passed', async () => {
        const title = await page.title()
        await expect(title).toBe('Google')
    })

    test('failed', async () => {
        const title = await page.title()
        await expect(title).toBe('')
    })

    afterAll(async () => {
        await page.close()
    });
})

So you got one failed test for chromium for example.

After it you can run only failed test with DEBUG flag:

DEBUG=true BROWSER=chromium jest -t 'failed'

So jest-playwright will run with some constant debug options:

            launchOptions: {
                headless: false,
                devtools: true,
            }

So you will be able to also run tests for failed describe block for example.

But you couldn't pass another playwright options with your CLI, I suppose it won't be critical

@jhildenbiddle
Copy link
Contributor Author

jhildenbiddle commented Jul 21, 2020

First, thank you @mmarkelov for your efforts. They are very much appreciated. Whatever we end up with, having the ability to modify configurations for describe and test blocks will be extremely helpful.

Both of the proposed implementations (new method & CLI option) are interesting because they serve different purposes: the config() method allows initiating debug mode for individual tests within test files while the CLI option allows scripts and IDE helpers to initiate debug mode for multiple describe and/or test blocks.

Implementation 1

test('test1', async () => {
  await jestPlaywright.config({
    // ...
  }, async ({page}) => {
    // ...
  })
});

This is essentially what I was hoping for. While it may be limited to single test blocks only, I still think this is approach has value because it allows me to run my tests in watch+headless mode, have a test fail, wrap the failed test in the code described above, then have access to a browser with dev tools ready for debugging. I didn't have to stop my watch task or deal with browser windows for every test after changing my main configuration. After I fix the test I just remove the jestPlaywright method and continue working.

As stated in the first comment though, I'd want to reuse a debug configuration automatically and simplify the code I need to wrap my tests with using a separate method. Passing a config is still useful in other scenarios though (like trying a different screen size or specifying a browser), so I can see value in both jestPlaywright.config (for adjusting the standard configuration per test) and a jestPlaywright.debug methods (for using a separate debug-focused config). Ideally the debug() method would also (optionally) accept a configuration as the first argument, allowing overrides of properties for the debug-focused config as well:

// Default debug configuration (stored in jest-playwright settings)
test('test1', async () => {
  await jestPlaywright.debug(async ({page}) => {
    // ...
  })
});

// Debug configuration with `browser` override
test('test2', async () => {
  await jestPlaywright.config({ browser: 'webkit' }, async ({page}) => {
    // ...
  })
});

// Regular configuration with `browser` override
test('test2', async () => {
  await jestPlaywright.config({ browser: 'chromium' }, async ({page}) => {
    // ...
  })
});

Other thoughts:

  • What is the value of this inside of the jestPlaywright method? My concern is that this is often referred to within tests to access things like the test title, so it's important to maintain the value of this from test() when executing test code inside of the jestPlaywright method.

  • If the page object is global and the config() method is synchronous (via await), why do we need to receive ({page}) inside the config method? Why can't we do this:

    await jestPlaywright.debug(async () => {
      await page.goto('https://google.com/');
      // ...
    })
  • Would it be possible to extend Jest's test object to allow something like this:

    // Defaults debug configuration
    test.jestPlaywrightDebug('test1', async () => {
      // ...
    });
    
    // Modified debug configuration
    test.jestPlaywrightDebug({ /* ... */ }, 'test2', async () => {
      // ...
    });
    
    // Modified standard configuration
    test.jestPlaywrightConfig({ /* ... */ }, 'test3', async () => {
      // ...
    });

Implementation 2

DEBUG=true BROWSER=chromium jest -t 'failed'

I would use this functionality to create VSCode debug scripts which already contain both the file context and (when selected) the describe/test name context, allowing me to click a button in VSCode to debug the current file or describe/test block without touching the CLI:

// .vscode/launch.json
{
  // Use IntelliSense to learn about possible attributes.
  // Hover to view descriptions of existing attributes.
  // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
  "version": "0.2.0",
  "configurations": [
    {
      "type": "node",
      "request": "launch",
      "name": "Debug current test file",
      "env": {
        "DEBUG": "true",
        "BROWSER": "chromium"
      },
      "runtimeExecutable": "npm",
      "runtimeArgs": [
        "run-script", 
        "test:jest"
      ],
      "args": [
        "--", 
        "-i", 
        "${relativeFile}",
      ],
      "console": "integratedTerminal"
    },
    {
      "type": "node",
      "request": "launch",
      "name": "Debug selected test name",
      "env": {
        "DEBUG": "true",
        "BROWSER": "chromium"
      },
      "runtimeExecutable": "npm",
      "runtimeArgs": [
        "run-script", 
        "test:jest"
      ],
      "args": [
        "--", 
        "-i", 
        "${relativeFile}",
        "-t",
        "${selectedText}"
      ],
      "console": "integratedTerminal"
    }
  ]
}

Obviously this hasn't been tested, but I don't see why it wouldn't work. A few additional thoughts:

  • I'm a macOS user so I can't confirm this, but I believe the CLI syntax above won't work on Windows. I'm sure there's a wy to accomplish the same thing, but I thought it was worth mentioning if/when docs are updated.

  • Using a generic DEBUG name for an environment variable could have unintended side effects. For example, Playwright already supports a DEBUG environment variable for debugging browser launches. Something more specific like JEST_PLAYWRIGHT_DEBUG would be safer even though it's more to type.

  • Where would the debug settings be stored? Can I modify them (I hope so), or are they fixed (aside from the BROWSER list shown in the example)? How would I set nested options like { launchOptions: { headless: false, devtools: true } } on the CLI? My preference would be define debug-focused options in jest.config.js (per Allow jest-playwright settings in jest.config.js #226) and just do this:

    JEST_PLAYWRIGHT_DEBUG=true jest -t 'failed'

Apologies for the lengthy post. Hope some of this is helpful. :)

@trusktr
Copy link

trusktr commented Jul 21, 2020

If the page object is global and the config() method is synchronous (via await), why do we need to receive ({page}) inside the config method? Why can't we do this:

Because if it shared the same page as with other tests, it would have to make the browser not headless for the particular test using jestPlaywright.debug. How would it dynamically make the browser non-headless for that test, then headless again after that test is finished?

My guess is that may not be possible without some OS-specific hacking beyond the current capabilities. The easy solution is to make a new non-headless browser instance, and give you that page.

test.jestPlaywrightDebug('test1', async () => {
  // ...
});

An API like that would be much more ergonomic! I'd say make it just test.playwrightDebug as we know we're in Jest already

@mmarkelov
Copy link
Member

@jhildenbiddle thank you for your comment. I think that we can start from the first implementation. It's almost done.
I think that it will be possible to use two helper functions: config and debug:

// Default debug configuration 
test('test1', async () => {
  await jestPlaywright.debug(async ({page}) => {
    // ...
  })
});

// Configuration will be used from the first argument
test('test2', async () => {
  await jestPlaywright.config({ ...options }, async ({page}) => {
    // ...
  })
});

Would it be possible to extend Jest's test object

Yeah, I agree that it will be much cleaner. But I'm not sure that it's possible to override test. I didn't find any way to do it. But I asked about this opportunity, so I'll make it if I can.

If we are talking about CLI implementation is about your playwright configuration. So you will be able to use just custom debug options. But I think that it will be possible to use jest.config.js or jest-playwright.config.js to pass them. Maybe something like this:

// jest-playwright.config.js
module.exports = {
  ...,
  debugOptions: { ... }
}

Something more specific like JEST_PLAYWRIGHT_DEBUG would be safer even though it's more to type.

Yeah, sure!

So to sum up, I will work on the first implementation. It will have two helper functions config and debug. For now you should use them inside test block, but if I find out any ways to extend test I will rewrite this in this way. The next step will be some additional research to use CLI for debugging, cause there is some extra question about this implementation

@jhildenbiddle
Copy link
Contributor Author

@mmarkelov

Excellent! I'm happy to test an in-progress branch before release if you'd like.

Thank you for all the hard work. Much appreciated.

@mmarkelov
Copy link
Member

@jhildenbiddle I just find out the way to extend global test. So there will be just two helpers on it:

// Defaults debug configuration
test.jestPlaywrightDebug('test1', async () => {
  // ...
});

// Modified standard configuration
test.jestPlaywrightConfig({ /* ... */ }, 'test2', async () => {
  // ...
});

I hope that we can merge the first version of it soon. But it will be a bit unstable - I need to implement some checks on these helpers and also I should check if test.jestPlaywrightConfig will work fine - it should override jest-playwright configuration

@mmarkelov
Copy link
Member

@jhildenbiddle And also I need to find out the way to run these kind of test only once if the browser param is passed:

// jest-playwright.config.js
module.exports = {
    ...,
    browsers: ['chromium', 'firefox'],
}

// test

// This test should run only once
test.jestPlaywrightConfig({ browser: 'chromium' }, 'test2', async () => {
  // ...
});

@mmarkelov
Copy link
Member

@jhildenbiddle just published new pre-release version. You can try jest-playwright-preset@next to use this helper functions. Also I suppose that we can simplify this two functions in one. Like this:

// Will use some custom debug options
test.jestPlaywrightDebug('test2', async () => {
  // ...
});

// Will use custom debug options and passed options
test.jestPlaywrightDebug({ ...options }, 'test2', async () => {
  // ...
});

@jhildenbiddle
Copy link
Contributor Author

jhildenbiddle commented Jul 22, 2020

@mmarkelov --

That's great. I'll check out the prerelease as soon as possible and post feedback here. In the meantime, a few thoughts...

Also I suppose that we can simplify this two functions in one.

I'm not sure which two functions your referring to, but if you're referring to test.jestPlaywrightConfig and test.jestPlaywrightDebug keep in mind that these serve different purposes (specifically, which jest-playwright configuration they use as their base):

// DEBUG without configuration object uses `debugOptions` config
test.jestPlaywrightDebug('test1', async () => {
  // ...
});

// DEBUG with configuration object merges (overrides) options with `debugOptions` config
test.jestPlaywrightDebug({ /* ... */ }, 'test2', async () => {
  // ...
});

// CONFIG must pass a configuration object and merges (overrides) options with primary config
test.jestPlaywrightConfig({ /* ... */ }, 'test3', async () => {
  // ...
});

Another thing to consider when extending test() is if/how .only() and .skip() are handled. Ideally, these same methods would be available on both jestPlaywrightConfig and jestPlaywrightDebug to allow for this:

// Run specific test(s) only
test.jestPlaywrightDebug.only('test1', async () => {
  // ...
});

// Skip specific test(s)
test.jestPlaywrightConfig.skip({ /* ... */ }, 'test2', async () => {
  // ...
});

If these methods cannot be added to the jestPlaywrightXXX methods, then perhaps only and skip options can be added to the configuration object to serve the same purpose:

// Run specific test(s) only
test.jestPlaywrightDebug({ only: true }, 'test1', async () => {
  // ...
});

// Skip specific test(s)
test.jestPlaywrightConfig({ skip: true }, 'test2', async () => {
  // ...
});

This is less than ideal because it presents a new method of isolating and skipping tests. Better to offer .only() and .skip() methods on the jestPlaywrightXXX options if possible.

@mmarkelov
Copy link
Member

@jhildenbiddle thank for getting back.

Another thing to consider when extending test() is if/how .only() and .skip() are handled.

I also will take it in, and try to implement it in next PRs

@mmarkelov
Copy link
Member

Another thing to consider when extending test() is if/how .only() and .skip() are handled. Ideally, these same methods would be available on both jestPlaywrightConfig and jestPlaywrightDebug

@jhildenbiddle I'm not sure that I can understand what we need to do for this cases

@jhildenbiddle
Copy link
Contributor Author

@mmarkelov --

Consider how devs are able to run or skip selected tests using test.only() or test.skip(). This same functionality--the ability to run or skip selected tests--should be available when using test.jestPlaywrightConfig() and test.justPlaywrightDebug() methods.

As stated above, ideally both the skip() and only() methods would be available on test.jestPlaywrightXXX methods:

// Run specific test(s) only
test.jestPlaywrightDebug.only('test1', async () => {
  // ...
});

// Skip specific test(s)
test.jestPlaywrightConfig.skip({ /* ... */ }, 'test2', async () => {
  // ...
});

This would require adding custom skip() and .only() methods to jestPlaywrightXXX because these methods need to handle a configuration object as the first argument for jestPlaywrightConfig and optionally as the first argument for jestPlaywrightDebug.

Hope that helps explain a bit better.

@mmarkelov
Copy link
Member

This would require adding custom skip() and only() methods to jestPlaywright

@jhildenbiddle just added this ability in next release

@jhildenbiddle
Copy link
Contributor Author

@mmarkelov. Amazing. Looking forward to checking it out. I've been busy setting up and configuring jest-image-snapshot for visual regression testing across platforms, but once that's in place I'll give the @next release a try.

As always, thanks for your efforts and the quick turnaround!

@jhildenbiddle
Copy link
Contributor Author

jhildenbiddle commented Jul 24, 2020

@mmarkelov --

Just gave jest-playwright-preset@next a test drive. Here's what I found:

  • Calling test.jestPlaywrightConfig() with a configuration did not have any effect (no browsers launched in non-headless mode).

    test.jestPlaywrightConfig(
      {
        browsers: ['chromium'],
        launchOptions: { headless: false },
      },
      'test name',
      async () => {
        /* ... */
      }
    );
    
  • Calling test.jestPlaywrightConfig.only() or test.jestPlaywrightConfig.skip() result in Invalid first argument errors. I'm assuming these methods just aren't implemented yet.

  • Calling test.jestPlaywrightDebug() without a configuration or debugOptions set had the following issue(s):

    1. Three browser windows opened: Chromium with devTools open, Nightly (Firefox) w/o devtools, and Playwright (Webkit) w/o devtools. I'm assuming this is due to a default debugOptions configuration? What are these default settings?
    2. All three open browsers were loaded with about:blank instead of the URL loaded in the test, so they failed to show the page content rendered in the test. The tests passed, so it seems like these browser windows just aren't properly associated with the tests. I am calling jestPlaywright.resetPage() using beforeEach(), so perhaps this is related?
    test.jestPlaywrightDebug(
      'test name',
      async () => {
        /* ... */
      }
    );
    
  • Calling test.jestPlaywrightDebug() with a configuration was the same as calling it without a configuration. The custom configuration I passed was ignored.

    test.jestPlaywrightDebug(
      {
        browsers: ['chromium'],
        launchOptions: { headless: false },
      },
      'test name',
      async () => {
        /* ... */
      }
    );
    
  • Adding debugOptions to jest-playwright.config.js had no effect on the behavior of test.jestPlaywrightDebug(). I would expect these options to serve as the base for all test.jestPlaywrightDebug() calls.

    // jest-playwright.config.js
    module.exports = {
      browsers: ['chromium', 'firefox', 'webkit'],
      debugOptions: {
        browsers: ['chromium'],
        launchOptions: {
          headless: false,
        },
      },
    };
  • Calling test.jestPlaywrightDebug.only() properly runs only the specified test.

  • Calling test.jestPlaywrightDebug.skip() properly skips the specified test.

  • The new methods falsely trigger some ESLint warnings/errors because test.jestPlaywright___() code is no longer seen as being nested inside of a test() block. For example, when using eslint-plugin-jest and eslint-plugin-jest-playwright all expect() statements inside test.jestPlaywrightConfig() and test.jestPlaywrightDebug() blocks produce the following warning/error by default:

    Expect must be inside of a test block. eslintjest/no-standalone-expect
    

@mmarkelov
Copy link
Member

@jhildenbiddle thank you for your feedback!

Calling test.jestPlaywrightConfig() with a configuration did not have any effect (no browsers launched in non-headless mode).

There is some problems with it right now. jest-playwright start browser with launchServer and for now I'm not sure that it will be able to configure it. Also jest-playwright starts separate playwright instance with debug and config helper functions. So you need pass launchType to make this start. IDK how to make it clear for now. Maybe I should make it underhood.

test.jestPlaywrightConfig(
  {
    browsers: ['chromium'],
    launchType: 'LAUNCH', 
    launchOptions: { headless: false },
  },
  'test name',
  async () => {
    /* ... */
  }
);

Calling test.jestPlaywrightConfig.only() or test.jestPlaywrightConfig.skip() result in Invalid first argument errors. I'm assuming these methods just aren't implemented yet.

This should work. I added it in rc9

    test.jestPlaywrightConfig.skip({
        /* your options here */
    }, 'failed', async ({page}) => {
        await page.goto('https://github.com/')
        const title = await page.title()
        await expect(title).toBe('Google')
    })

Three browser windows opened: Chromium with devTools open, Nightly (Firefox) w/o devtools, and Playwright (Webkit) w/o devtools. I'm assuming this is due to a default debugOptions configuration? What are these default settings?

It's using this config:

const DEBUG_OPTIONS = {
  launchType: 'LAUNCH',
  launchOptions: {
    headless: false,
    devtools: true,
  },
}

All three open browsers were loaded with about:blank instead of the URL

You should use page from arguments

    test.jestPlaywrightDebug('failed', async ({page}) => {
        await page.goto('https://github.com/')
        const title = await page.title()
        await expect(title).toBe('Google')
    })

Calling test.jestPlaywrightDebug() with a configuration was the same as calling it without a configuration. The custom configuration I passed was ignored.

I'll check it

Adding debugOptions to jest-playwright.config.js had no effect on the behavior of test.jestPlaywrightDebug(). I would expect these options to serve as the base for all test.jestPlaywrightDebug() calls.

Yeah! There is no implementation to support debugOptions in jest-playwright.config.js for a while

Calling test.jestPlaywrightDebug.only() properly runs only the specified test.

Calling test.jestPlaywrightDebug.skip() properly skips the specified test.

This is expected behavior. Is it wrong?

The new methods falsely trigger some ESLint warnings/errors because

I will take a look

@creage
Copy link

creage commented Nov 5, 2020

@mmarkelov, @jhildenbiddle
Hey mates, sorry for interrupting your discussion, but to me the entire flow looks a bit broken. I mean, you should not modify your code to enter debug mode. You should probably modify it when you actually want to fix it.

Saying that, implementing most of the API presented here is a bit overkill to me.

Let me explain how could one address the actual issue in the OP.

Instead of keeping all of your test run parameters in jest-playwright.config.js, you should keep some/most of them in some JSON file, which will present your environment.

{
	"APP_PROTOCOL": "http:",
	"APP_HOST": "app.in.test",
	"APP_PATH": "app/client",

	"SLOW_MO": 0,
	"VIEWPORT": "1920x1080",

	"HEADLESS": true,
	"SCREEN_FAILED_TESTS": true,
	"DEFAULT_TIMEOUT": 600,
	"RETRY_TIMES": 3,

	"TYPING_DELAY": 0,

	"BROWSERS": "chromium,firefox,webkit",
	"DEVICES": "",
	"TEST_NAME_PATTERN": ""
}

And you can read that JSON in your jest-playwright.config.js, and set it's values as env vars, but only those that are not yet defined:

const defaults = require('./tests.config.json');
// push config variables to env
Object.keys(defaults).forEach(key => {
	if (!process.env[key]) {
		process.env[key] = defaults[key];
	}
});
// here you consume your env vars, something like
const [width, height] = (process.env.VIEWPORT || '1920x1080').split('x').map(v => +v);

module.exports = {
	launchOptions: {
		headless: process.env.HEADLESS === 'true',
		slowMo: +process.env.SLOW_MO
	},
	contextOptions: {
		ignoreHTTPSErrors: true,
		bypassCSP: true,
		viewport: {
			width,
			height
		}
	},	
	browsers: process.env.BROWSERS ? process.env.BROWSERS.split(',') : ['chromium'],
	devices: process.env.DEVICES ? process.env.DEVICES.split(',') : []
};

This approach gives you an ability to set env vars for you particular test run, and run your tests with no code modification.

set BROWSERS=chromium
set HEADLES=false
set RETRY_TIMES=0
jest -t "my test name I want to run"

So far I made zero changes to run my particular test with particular parameters. And you can parametrize your setup up to your needs.

Again, if you want to skip a test - can't you use Jest's it.skip(), and it.only() for selected run? Why you introduce new hooks in this runner?

And in order to inject a breakpoint, since you mentioned VSCode - it has super nice JavaScript Debug Terminal, which attaches to any Node code/process, and you can place your breakpoints directly in your test sources. No need for explicit debugger statements or similar.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants