Skip to content

First stab at adding support for setup() and teardown()#65

Merged
DmitrySharabin merged 4 commits intomainfrom
setup-teardown
Jan 22, 2025
Merged

First stab at adding support for setup() and teardown()#65
DmitrySharabin merged 4 commits intomainfrom
setup-teardown

Conversation

@DmitrySharabin
Copy link
Copy Markdown
Member

I still need to add some docs, but I want to ensure I get the idea right.

My local experiments with another project show that those inheritable methods work.

@LeaVerou, I'd love to hear your thoughts on this.

@netlify
Copy link
Copy Markdown

netlify bot commented Jan 22, 2025

Deploy Preview for h-test ready!

Name Link
🔨 Latest commit 098c644
🔍 Latest deploy log https://app.netlify.com/sites/h-test/deploys/67917e72ae3b830008f3516c
😎 Deploy Preview https://deploy-preview-65--h-test.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment thread src/classes/TestResult.js Outdated
Copy link
Copy Markdown
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

LGTM. Though it might be a simpler design to just define no-op setup() and teardown() methods (so there's always something there) and then just assign any user-provided ones to this.setup/this.teardown. I think it would lead to strictly less code, since we now have two methods that do nothing more than call two other methods :P Even the context is the same.

Co-authored-by: Lea Verou <lea@verou.me>
@DmitrySharabin
Copy link
Copy Markdown
Member Author

DmitrySharabin commented Jan 22, 2025

LGTM. Though it might be a simpler design to just define no-op setup() and teardown() methods (so there's always something there) and then just assign any user-provided ones to this.setup/this.teardown. I think it would lead to strictly less code, since we now have two methods that do nothing more than call two other methods :P Even the context is the same.

That's an excellent idea!

I have a question, though. I used this.setup/this.teardown to promisify setup() and teardown() so we can chain them with this.run. Should I do the same in the design you proposed? Do we need a utility to promisify a function? Or am I doing something wrong here?

UPDATE

I think I know the answer: we can simply do Promise.resolve(this.test.setup()). I'll commit the change after testing it.

@DmitrySharabin DmitrySharabin marked this pull request as ready for review January 22, 2025 22:27
@DmitrySharabin
Copy link
Copy Markdown
Member Author

Thank you for your suggestions on how to improve the design. 🙏 Now, the code is much-much simpler.
Could you please take another look to make sure I got everything correctly?

Comment thread src/classes/Test.js Outdated
Co-authored-by: Lea Verou <lea@verou.me>
@DmitrySharabin DmitrySharabin merged commit 4ab9b8f into main Jan 22, 2025
@DmitrySharabin DmitrySharabin deleted the setup-teardown branch January 22, 2025 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants