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

add chained tests #136

Merged
merged 10 commits into from
Dec 31, 2024
Merged

add chained tests #136

merged 10 commits into from
Dec 31, 2024

Conversation

crabest
Copy link
Contributor

@crabest crabest commented Dec 24, 2024

For #123, I thought of adding chained tests just like a testA inside testB to let testA depends on testB, but with this approach testA cannot depend on another testC unless testC is between testA and testB.
As a solution we can let the tests seperated but it can depend on multiple tests no matter the position.

Please let me know your thoughts and feedback.

@CLAassistant
Copy link

CLAassistant commented Dec 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

vercel bot commented Dec 24, 2024

@crabest is attempting to deploy a commit to the Antiwork Team on Vercel.

A member of the Team first needs to authorize it.

@crabest crabest marked this pull request as draft December 24, 2024 13:06
packages/shortest/README.md Outdated Show resolved Hide resolved
@crabest
Copy link
Contributor Author

crabest commented Dec 25, 2024

for less confusion, I named it afterTest() that can accept more than one shortest variables

@crabest crabest marked this pull request as ready for review December 25, 2024 12:13
@slavingia
Copy link
Contributor

slavingia commented Dec 25, 2024

I prefer "after", as it's... shorter!

@crabest
Copy link
Contributor Author

crabest commented Dec 25, 2024

I said for less confusion because we have after() method for shortest for callback functions
image

what do you think

@slavingia
Copy link
Contributor

Hm where's that coming from, if it's not being imported. But good point, I'll think on it

@slavingia
Copy link
Contributor

Could we overload it? So you could use both?

@crabest
Copy link
Contributor Author

crabest commented Dec 25, 2024

we can use both, the "less confusion" just for us humans

@slavingia
Copy link
Contributor

Let's go with after. I think the simple language is critical for adoption.

@crabest
Copy link
Contributor Author

crabest commented Dec 25, 2024

renamed to "after", now it should be good!

Copy link

vercel bot commented Dec 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shortest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 30, 2024 8:32pm

Copy link
Contributor

@slavingia slavingia left a comment

Choose a reason for hiding this comment

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

Commented. Feature itself looks good! I do think we should update this repo with at least one example that can be run locally and in CI.

packages/shortest/README.md Outdated Show resolved Hide resolved
packages/shortest/README.md Outdated Show resolved Hide resolved
packages/shortest/README.md Outdated Show resolved Hide resolved
packages/shortest/README.md Outdated Show resolved Hide resolved
packages/shortest/README.md Outdated Show resolved Hide resolved
@slavingia
Copy link
Contributor

slavingia commented Dec 27, 2024

ideas from internal discussion:

const login = shortest('user can login with email and password')
shortest(login).shortest('user can modify their account-level refund policy from the default of 30-days to no refunds allowed')

const loginAsLawyer = shortest('...')
const loginAsContractor = shortest('...')
const allAppActions = [shortest('send invoice to company'), shortest('view invoices')];
shortest(loginAsLawyer.allAppActions)
shortest(loginAsContractor.allAppActions)

This would make it even shorter.

@crabest
Copy link
Contributor Author

crabest commented Dec 27, 2024

let me in those discussions (:
for the array part, this would be shorter and more readable.

const allAppActions = shortest(['send invoice to company', 'view invoices'])

@slavingia
Copy link
Contributor

slavingia commented Dec 27, 2024

agreed! sorry, just a daily catchup call we do for our products; I do want to move them to being public! next week! 10am EST Friday

@crabest
Copy link
Contributor Author

crabest commented Dec 27, 2024

10am EST Friday, sadly I would still be at work.
for those changes, let's iterate in the same pull request? what do you think

@slavingia
Copy link
Contributor

yep sounds good!

@crabest
Copy link
Contributor Author

crabest commented Dec 27, 2024

with those changes, I think let's drop that after()

@crabest crabest marked this pull request as draft December 27, 2024 18:29
@crabest
Copy link
Contributor Author

crabest commented Dec 27, 2024

using your example, this is how it will look like now:

const login = 'user can login with email and password'
shortest([login, 'user can modify their account-level refund policy from the default of 30-days to no refunds allowed'])

const loginAsLawyer = '...'
const loginAsContractor = '...'
const allAppActions = ['send invoice to company', 'view invoices'];
shortest([loginAsLawyer, ...allAppActions])
shortest([loginAsContractor, ...allAppActions])

@slavingia
Copy link
Contributor

Neato! @rmarescu @m2rads et al, happy w/ that?

@crabest crabest marked this pull request as ready for review December 27, 2024 22:50
@m2rads
Copy link
Contributor

m2rads commented Dec 29, 2024

Seems like your new changes are not resolving properly.

image

Here's how you can fix it:

When making changes to src/index.ts you'd need to update index.d.ts as well.
We have been dealing with some issues when generating index.d.ts when bundling the package so we implement it manually. Please make sure your new changes are reflected in that file as well.

Here's why this issue happens:

src/index.ts is our source file that contains the actual implementation and gets compiled to JavaScript.

index.d.ts is our declaration file that only contains type definitions and Is used by TypeScript for type checking. It also defines the public API types for consumers.

We need both because the compiled JavaScript (dist/index.js) doesn't contain type information and external users of our package need type definitions for TypeScript support.

I created a new issue: #205 to fix it.
Please let me know if you have any questions regarding this part.

@m2rads
Copy link
Contributor

m2rads commented Dec 29, 2024

Would be good to have a demo using this new feature when you have finished your implementation.

@crabest
Copy link
Contributor Author

crabest commented Dec 29, 2024

const login = 'user can login with email and password'
shortest([login, 'user can modify their account-level refund policy from the default of 30-days to no refunds allowed'])

const loginAsLawyer = '...'
const loginAsContractor = '...'
const allAppActions = ['send invoice to company', 'view invoices'];
shortest([loginAsLawyer, ...allAppActions])
shortest([loginAsContractor, ...allAppActions])

the example you used is not implemented, only the example above ^

@m2rads
Copy link
Contributor

m2rads commented Dec 29, 2024

Looks good to me. Would be nice to write some new tests using the new chained tests.

To prepare for release please update your branch and bump the version to 0.3.1. I think this version is appropriate as this PR is a major feature. Also please update changelogs to add your changes.

@slavingia
Copy link
Contributor

0.3.0 imo

@m2rads
Copy link
Contributor

m2rads commented Dec 29, 2024

Can we export a test and use it in another test file?

@m2rads
Copy link
Contributor

m2rads commented Dec 30, 2024

Looks good to me. Would be nice to write some new tests using the new chained tests.

To prepare for release please update your branch and bump the version to 0.3.1. I think this version is appropriate as this PR is a major feature. Also please update changelogs to add your changes.

@crabest You don't need to do this anymore. Once review is done will just merge to main.

@slavingia
Copy link
Contributor

Please merge main to include a new caching feature!

@crabest
Copy link
Contributor Author

crabest commented Dec 30, 2024

Done!

Copy link
Contributor

@slavingia slavingia left a comment

Choose a reason for hiding this comment

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

Lgtm

README.md Outdated Show resolved Hide resolved
Co-authored-by: Sahil Lavingia <[email protected]>
@PedroAVJ
Copy link

Great work guys! Was just looking to do a pattern like this for longer test workflows

@slavingia
Copy link
Contributor

Made one change: f7dc8df

IMO, we should try to avoid "expect" as that's a pre-AI approach to writing tests. The AI can determine this and fail the test of any of the other test conditions (in natural language) failed.

@slavingia
Copy link
Contributor

If you agree, please merge or let me know and I can do so!

@crabest
Copy link
Contributor Author

crabest commented Dec 31, 2024

sure, we just need to adjust the the prompt to let it understand to get the expectation the magic string if we didn't pass those exceptions.

@crabest
Copy link
Contributor Author

crabest commented Dec 31, 2024

you can merge, I can't!

@slavingia slavingia merged commit c148467 into anti-work:main Dec 31, 2024
2 of 4 checks passed
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.

6 participants