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

Adding BeforeAll and AfterAll hooks #1114

Merged
merged 7 commits into from
Nov 8, 2023
Merged

Conversation

VidhiRambhia
Copy link

@VidhiRambhia VidhiRambhia commented Nov 6, 2023

This PR adds BeforeAll(..) and AfterAll(..) hooks, fixes #758

@VidhiRambhia VidhiRambhia mentioned this pull request Nov 6, 2023
3 tasks
@VidhiRambhia VidhiRambhia marked this pull request as ready for review November 7, 2023 15:44
})
"""
When I run cypress
Then it passes
Copy link
Owner

Choose a reason for hiding this comment

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

This test can be moved and incorperated into features/hooks_ordering.feature. Issue-specific tests are only for behavior that is otherwise hard to describe.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -18,7 +18,7 @@ import DataTable from "./data_table";

import {
assignRegistry,
freeRegistry,
freeRegistry, getRegistry,
Copy link
Owner

Choose a reason for hiding this comment

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

You should run all tests, this will reveal this as a problem (unused variable and incorrect formatting).

$ npm run test

Copy link
Author

Choose a reason for hiding this comment

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

Got it, will remove this

@@ -775,6 +780,31 @@ function beforeHandler(context: CompositionContext) {
taskSpecEnvelopes(context);
}

function beforeAllHandler(this: Mocha.Context, context: CompositionContext) {
Copy link
Owner

Choose a reason for hiding this comment

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

This method's content can be moved into beforeHandler(..).

Copy link
Author

Choose a reason for hiding this comment

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

Refactored it

const { registry } = context;
let beforeAllHooks = registry.resolveBeforeAllHooks();
for(const beforeAllHook of beforeAllHooks) {
if (beforeAllHook) {
Copy link
Owner

Choose a reason for hiding this comment

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

This check seems unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's redundant. Removing it

.then((start) => {
const end = createTimestamp();
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

Wrapping runStepWithLogGroup in order to obtain start- & end-time is only necessary when these values are actually used for something and this isn't the case here. Thus, you can replace

cy.then(() => {
  const start = createTimestamp();
  return cy.wrap(start, { log: false });
})
  .then((start) => {
    runStepWithLogGroup({
      fn: () => registry.runHook(this, hook),
      keyword: "BeforeAll"
    });

    return cy.wrap(start, { log: false });
  })
  .then((start) => {
    const end = createTimestamp();
  });

.. with

runStepWithLogGroup({
  fn: () => registry.runHook(this, hook),
  keyword: "BeforeAll"
});

Copy link
Author

Choose a reason for hiding this comment

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

I see. Made this change

.then((start) => {
const end = createTimestamp();
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same goes for here.

@@ -127,6 +127,28 @@ function defineAfterStep(
}
}

function defineBeforeAll(fn: IHookBody): void;
function defineBeforeAll(
maybeFn?: IHookBody
Copy link
Owner

Choose a reason for hiding this comment

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

The optionality and function overloading here is unecessary, as there's only one way to invoke this method.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I'll change this here and in other places too


function defineAfterAll(fn: IHookBody): void;
function defineAfterAll(
maybeFn?: IHookBody
Copy link
Owner

Choose a reason for hiding this comment

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

Same goes for here.

export function BeforeAll(fn: IStepHookBody): void;
export function BeforeAll(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
maybeFn?: IStepHookBody
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

export function AfterAll(fn: IStepHookBody): void;
export function AfterAll(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
maybeFn?: IStepHookBody
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

Copy link
Author

Choose a reason for hiding this comment

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

Made the change

lib/registry.ts Outdated
@@ -101,7 +101,6 @@ export class Registry {
this.runStepDefininition = this.runStepDefininition.bind(this);
this.defineParameterType = this.defineParameterType.bind(this);
this.defineBefore = this.defineBefore.bind(this);
this.defineAfter = this.defineAfter.bind(this);
Copy link
Owner

Choose a reason for hiding this comment

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

This is probably not right.

Copy link
Author

Choose a reason for hiding this comment

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

Deleted this by mistake, reverting it back

@@ -25,6 +25,8 @@ import {
After,
BeforeStep,
AfterStep,
BeforeAll,
AfterAll,
Copy link
Owner

@badeball badeball Nov 8, 2023

Choose a reason for hiding this comment

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

You can add the same tests here, as you have in test-d/entrypoint-browser.test-d.ts.

badeball added a commit that referenced this pull request Nov 8, 2023
This primarily renames a bunch of stuff to more clearly differentiate
between the three types of hooks that exists now, namely:

 - run hooks (BeforeAll, AfterAll)
 - case hooks (Before, After)
 - step hooks (BeforeStep, AfterStep)

The naming convention comes from cucumber-js [1].

Additionally, the order of AfterAll is reversed before execution.

Lastly, documentation has been updated.

[1] https://github.com/cucumber/cucumber-js/blob/v10.0.0/src/support_code_library_builder/index.ts#L72-L77
badeball added a commit that referenced this pull request Nov 8, 2023
This primarily renames a bunch of stuff to more clearly differentiate
between the three types of hooks that exists now, namely:

 - run hooks (BeforeAll, AfterAll)
 - case hooks (Before, After)
 - step hooks (BeforeStep, AfterStep)

The naming convention comes from cucumber-js [1].

Additionally, the order of AfterAll is reversed before execution.

Lastly, documentation has been updated.

[1] https://github.com/cucumber/cucumber-js/blob/v10.0.0/src/support_code_library_builder/index.ts#L72-L77
@badeball badeball merged commit c5f4c8a into badeball:master Nov 8, 2023
6 checks passed
@badeball
Copy link
Owner

badeball commented Nov 8, 2023

Excellent, thank you. I did some clean up in 61623bc, some of that stuff was just easier to do myself than to explain.

@VidhiRambhia
Copy link
Author

Thank you, @badeball!
Please let me know if I can contribute to some other issue

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.

Add 'BeforeAll' hook
2 participants