Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Allow to override chai from the config #5974

Merged
merged 2 commits into from Jun 15, 2023

Conversation

farcaller
Copy link
Contributor

@farcaller farcaller commented Mar 17, 2023

PR description

Allow to override the chai instance from the config. This allows one to reduce the test boilerplate when chai plugins are used, e.g. this snippet could move to the config, instead of being repeated in every test:

import chai from 'chai';
import chaiAsPromised from 'chai-as-promised';

chai.use(chaiAsPromised);

const { assert } = chai;

Testing instructions

require chai from the config and point config's chaiOverride to it. No default value is offered as the default is undefined.

Documentation

Trivial change, no docs required.

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

Non-breaking.

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @farcaller ,

Thanks for the PR. I have a question.

Test instructions
require chai from the config and point config's chaiOverride to it. No default value is offered as the default is undefined.

Can you explain what chaiOverride is, and which config it resides in?

@farcaller
Copy link
Contributor Author

my understanding is that config is resolved to the truffle-config.js, so you can set up the chai import in there.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding is that config is resolved to the truffle-config.js, so you can set up the chai import in there.

This PR introduces behavior based on a currently undocumented config setting. I think this PR should include the new setting, along with its a default value, so the complete feature implementation is captured in one place.

Thoughts, @eggplantzzz, @gnidan?

@gnidan
Copy link
Contributor

gnidan commented Apr 4, 2023

Sorry for the delay. Just getting back to this.

I think this approach generally is fine, i.e., allowing users to specify a different package name in the truffle-config.

I think we should do something like this, though, instead:

module.exports = {
  // ... rest of truffle-config.js

  chai: {
    package: "chai-as-promised"
  }
}

I think this might be the sort of thing we need to add to @truffle/config itself, also, but that sounds like more of an @eggplantzzz question.

Oh, maybe package isn't the best name... alternatives include packageName or require.

@eggplantzzz
Copy link
Contributor

Hey @farcaller, so if you can make the changes that @gnidan suggested we can move forward on this. The important part here is that the property in the truffle-config.js should be chai.package instead of chaiOverride. It seems like this should probably be enough for now. Currently there really isn't a need to have default values for this in @truffle/config, unless there is something I missed @gnidan.

Let us know if you need anything else! Also, if you feel like you won't have time to finish this, @cds-amal says he can bring it across the finish line. Just let us know :)

@eggplantzzz
Copy link
Contributor

So @farcaller, does the thumbs up mean you'd like to bring this across the finish line? Let us know either way. Like I said, we can take it up if you don't have the time. Thanks!

@farcaller
Copy link
Contributor Author

farcaller commented Apr 25, 2023

The thumbs up is my seeing the feedback :) I’m out on holidays this whole month and will get back to coding in May. Sorry for the delays!

@eggplantzzz
Copy link
Contributor

Ok great! Enjoy your vacation!

@cliffoo
Copy link
Contributor

cliffoo commented Jun 8, 2023

Hey @farcaller are you still interested in putting the rest of the PR together? Let us know, thanks!

@farcaller
Copy link
Contributor Author

Sorry for the [extreme] delay. The PR is updated as per the feedback.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, so as this currently is it actually has the user doing the require in the config rather than just supplying the package name... is that what we want?

@gnidan
Copy link
Contributor

gnidan commented Jun 14, 2023

Huh, so as this currently is it actually has the user doing the require in the config rather than just supplying the package name... is that what we want?

This is definitely more ideal, where @truffle/test's internals should do the require() for the user.

But this might run into problems... if @truffle/test does require("chai"), it will likely use ./node_modules/@truffle/test/node_modules/chai, when the user probably wants ./node_modules/chai.

So if we want to avoid this precedence concern, the way to do that is to put the require("chai") inside truffle-config.js.

I think this might be what we're stuck with. Thanks for raising this concern!

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following through on this @farcaller!

@gnidan gnidan added enhancement doc-change-required Issue indicates a change to documentation is required new-feature: @truffle/test labels Jun 14, 2023
Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if this new way is good, then I'll approve it too!

@haltman-at haltman-at merged commit f580f0c into trufflesuite:develop Jun 15, 2023
10 checks passed
@farcaller farcaller deleted the patch-1 branch June 15, 2023 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
doc-change-required Issue indicates a change to documentation is required enhancement new-feature: @truffle/test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants