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

Improve module environment inheritance with a Object.create like #869

Closed
leobalter opened this issue Oct 5, 2015 · 7 comments
Closed

Improve module environment inheritance with a Object.create like #869

leobalter opened this issue Oct 5, 2015 · 7 comments
Labels
Status: Declined Status: Ready A "Meta" type issue that has reached consensus. Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.

Comments

@leobalter
Copy link
Member

Ref #859 (comment)

Instead of using QUnit's extend method to extend a module's env. inheritance, we should improve it with a Object.create's like inheritance.

Suggested by @gibson042:

var createObject = (function( Base ) {
    return function( prototype ) {
        Base.prototype = prototype;
        return new Base();
    };
})( Function() );

env = createObject( parentModule.testEnvironment );

We could also use Object.create adding a polyfill that would be used only on IE6~IE8 for the supported browsers.

@jzaefferer
Copy link
Member

Sounds good to me. Would help to see an actual implementation.

@jzaefferer
Copy link
Member

For the Object.create polyfill, we can inline that without the comments, but with a // Support ... comment.

@leobalter
Copy link
Member Author

I want to investigate using LoDash's _.create as we might use _.isEqual in #865

@jzaefferer
Copy link
Member

Good idea. It'll be a while until I can get back to working on the build to include lodash modules, so feel free to take that over.

Maybe we should start by trying to switch to ES6 modules (again), with rollup? Then add more dependencies from there.

Or maybe using webpack? I had good experience with that on other projects. It would certainly deal well with a mix of internal ES6 modules and external modules, in whatever format.

@leobalter leobalter added the Status: Ready A "Meta" type issue that has reached consensus. label Oct 21, 2015
@trentmwillis
Copy link
Member

Is this still desired? Since we now support IE9+ we could use Object.create directly, but I'm unsure what benefit this gives us over using the current extend implementation.

@gibson042
Copy link
Member

The biggest reason for this issue was ensuring that every test has a distinct environment based on its module's environment and shared with the entire before/beforeEach queue and afterEach/after stack, and that is already fulfilled per #859 (comment) . There's still value in actual inheritance over shallow copying, but I think it's exclusive to sugar for advanced asynchronous use cases (and "sugar" in the sense that they can still work without it). For example:

QUnit.module("outer", function( outerModule ) {
	// Load an ES2018 module into this environment, asynchronously
	// cf. https://github.com/tc39/proposal-dynamic-import
	this.dynamicModulePromise = import("./dynamicModule.js")
		.then( module => this.dynamicModule = module );

	QUnit.module("doesntNeedDynamicModule", function( innerModule ) {
		QUnit.test();
	});
	QUnit.module("needsDynamicModule", function( innerModule ) {
		innerModule.before( () => await this.dynamicModulePromise );
		QUnit.test();
	});
	QUnit.module("alsoNeedsDynamicModule", function( innerModule ) {
		innerModule.before( () => await this.dynamicModulePromise );
		QUnit.test();
	});
});

I'm cool with closing it.

@Krinkle Krinkle added Type: Enhancement New idea or feature request. task labels Oct 20, 2017
@Krinkle Krinkle added Type: Meta Seek input from maintainers and contributors. and removed task labels Dec 22, 2018
@Krinkle
Copy link
Member

Krinkle commented Jun 25, 2020

Closing for now. Not having live inheritence here I think actually makes the test runner slightly easier to reason about, and would also be a theoretically breaking change that will be hard to explain.

Having said that, for future readers: There is no strong opposition to this, so if you find a particular use case is impossible or very hard without this, feel free to create a new issue and we'd likely be open to reconsidering it and/or accepting a patch :)

@Krinkle Krinkle closed this as completed Jun 25, 2020
raycohen added a commit to raycohen/qunit that referenced this issue Mar 4, 2021
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
raycohen added a commit to raycohen/qunit that referenced this issue Mar 5, 2021
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
raycohen added a commit to raycohen/qunit that referenced this issue Mar 20, 2021
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
raycohen added a commit to raycohen/qunit that referenced this issue Jun 9, 2024
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
raycohen added a commit to raycohen/qunit that referenced this issue Jun 14, 2024
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
raycohen added a commit to raycohen/qunit that referenced this issue Jun 21, 2024
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Fixes qunitjs#1328.
Ref qunitjs#869.
Krinkle pushed a commit to raycohen/qunit that referenced this issue Jun 23, 2024
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Once before hooks have run, create a flattened deep copy of the module
testEnvironment and assign that to test testEnvironment. At this point nothing
should use test.testEnvironment until the before hooks have run.

Fixes qunitjs#1328.
Ref qunitjs#869.
Krinkle pushed a commit that referenced this issue Jun 25, 2024
The before and after hooks run once per module as long as there is at least one
test in the module. Using environment inheritance allows us to use the module
context in those hooks, which allows reading the expected changes to the
context from a before hook inside nested modules.

Once before hooks have run, create a flattened deep copy of the module
testEnvironment and assign that to test testEnvironment. At this point nothing
should use test.testEnvironment until the before hooks have run.

Fixes #1328.
Ref #869.
Closes #1762.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Declined Status: Ready A "Meta" type issue that has reached consensus. Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

No branches or pull requests

5 participants