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

[BC Idea]: BCPT Handled event in AddLogEntry #437

Closed
NAVFreak opened this issue Dec 16, 2023 · 8 comments · Fixed by #495
Closed

[BC Idea]: BCPT Handled event in AddLogEntry #437

NAVFreak opened this issue Dec 16, 2023 · 8 comments · Fixed by #495
Labels
Approved The issue is approved BCIdea Issue related to a BCIdea

Comments

@NAVFreak
Copy link
Contributor

NAVFreak commented Dec 16, 2023

BC Idea Link

https://experience.dynamics.com/ideas/idea/?ideaid=15563e0c-b492-ee11-a81c-000d3a7e6e50

Description

In codeunit 149005 "BCPT Line" , there is the function:

 internal procedure AddLogEntry(var BCPTLine: Record "BCPT Line"; Operation: Text; ExecutionSuccess: Boolean; Message: Text; NumSQLStatements: Integer; StartTime: DateTime; EndTime: Datetime)

I would like that we put a handled integration event in the beginning of this function.

OnBeforeAddLogEntry(BCPTLine, Operation, ExecutionSuccess, Message, handled);

    if Handled then
      exit;
...
// Event code (wrong copy/paste in bc idea)

 [IntegrationEvent(false, false)]
    local procedure OnBeforeAddLogEntry(var BCPTLine: Record "BCPT Line"; var Operation: Text; var ExecutionSuccess: Boolean; var Message: Text; var Handled: Boolean)
    begin
    end;

Reason:
Many of us run BCPT flow with random customer data to get a better representation of the data that is used.
The problem with doing that is sometimes some items/customers/vendors have very special setting that makes a confirmation or message popup. In order to tackle that we do message/confirmation handlers.

But since this does not always occur we get an error for every time we didn't use the handler. An event like this can give us the possibility to skip that error if we want it.

I have seen some argument that is up to the developer to create clean test cases where the handlers are always used. To some extent that is valid. But the other side of the coin is that we have to do a lot of code cloning to make those test distinct. But the worst part is that will pollute the the actual performance test. Because now we have to filter out the records which has these particular settings. Settings that wasn't meant to be filtered upon. This will lead to that the slowest functions are those that prepares the data in the test and that makes the performance test quite useless since we can't measure the realistic flows.

@NAVFreak NAVFreak added the BCIdea Issue related to a BCIdea label Dec 16, 2023
@ChethanT
Copy link
Contributor

ChethanT commented Jan 8, 2024

@NAVFreak do I understand correctly that we need this event to supress or ignore the test failures in the log?
I feel a better solution would be to have a test runner which can be setup to ignore these handler errors? Let me talk to our architects and get back to you.

@JesperSchulz JesperSchulz added the Follow Up The issue has an open question and must be followed up on label Jan 8, 2024
@NAVFreak
Copy link
Contributor Author

NAVFreak commented Jan 8, 2024

Yes since some handler functions might or might not be run due to random usage of data.

One argument that it should be an event is that we can still decide which handler events that we want to bypass.
In one test we want it inactivted while in another test we would like to know if there are deprecated handlers that are not used anymore. That has happened to when I have updated the customer process but don't know which handler events that was for the previous code and is now deprecated "dead code"

@ChethanT
Copy link
Contributor

ChethanT commented Jan 9, 2024

If you want a test not to fail on unhandled messagehandler, can't you just open a message explicitly in the test? It is not super nice but a quick fix.

@NAVFreak
Copy link
Contributor Author

NAVFreak commented Jan 9, 2024

I don't understand what you mean.
Currently I have built my own modified version of BCPT to deal with this but since that is a bad practice/quick fix I try to move the modifications here 😉

@ChethanT
Copy link
Contributor

ChethanT commented Jan 10, 2024

I mean
[Test]
[HandlerFunctions('MessageHandler')]
[Scope('OnPrem')]
procedure BusinessScenario()
begin
...
...
...
MethodShowsMessageSometimes();
...
...
...
// Explicitly open a Message to make sure 'handler not run' error does not happen.
Message('Test');
end;

Another solution would be to add capability to the BCPTTestContext to ignore message or confirm handler not being called.

@NAVFreak
Copy link
Contributor Author

NAVFreak commented Jan 10, 2024 via email

@ChethanT
Copy link
Contributor

Based on our discusion, it looks like raising an event gives us more flexibility other solutions we dicussed. Lets approve them:-)

@ChethanT ChethanT added Approved The issue is approved and removed Follow Up The issue has an open question and must be followed up on labels Jan 11, 2024
NAVFreak pushed a commit to NAVFreak/BCApps that referenced this issue Jan 18, 2024
NAVFreak pushed a commit to NAVFreak/BCApps that referenced this issue Jan 19, 2024
Exposes an event to make it possible to skip errors from unhandled handler functions.
@NAVFreak
Copy link
Contributor Author

@ChethanT PR is ready

JesperSchulz added a commit that referenced this issue Feb 26, 2024
<!-- Thank you for submitting a Pull Request. If you're new to
contributing to BCApps please read our pull request guideline below
*
[https://github.com/microsoft/BCApps/Contributing.md](https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FBCApps%2FContributing.md&data=05%7C02%7CMarcus.Nordlund%40navet.se%7C20e35a07443c45d23d3408dc187791b1%7C56bbfa1c071347e3957f22760ff6118d%7C0%7C0%7C638412149035646962%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=8CDDm%2F8UKghOORGdxlDrP3tmCRQnHAcOx8UNikZIK3o%3D&reserved=0)
-->
#### Summary <!-- Provide a general summary of your changes -->
This PR exposes an event to make it possible to skip an unwanted error
when running a BCPT suit.
The idea was to expose an event i BCPTLine codeunit.

However, during the implementation I discovered that [BCPTLine codeunit
has access internal]
([https://github.com/microsoft/BCApps/blob/faeca5adabaa2e2b7644d744e21f9a1f97d1d971/src/Tools/Performance%20Toolkit/App/src/BCPTLine.Codeunit.al#L12](https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FBCApps%2Fblob%2Ffaeca5adabaa2e2b7644d744e21f9a1f97d1d971%2Fsrc%2FTools%2FPerformance%2520Toolkit%2FApp%2Fsrc%2FBCPTLine.Codeunit.al%23L12&data=05%7C02%7CMarcus.Nordlund%40navet.se%7C20e35a07443c45d23d3408dc187791b1%7C56bbfa1c071347e3957f22760ff6118d%7C0%7C0%7C638412149035646962%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ojrRAsw6wNK0d%2BBlFSVyE4U3I0p6fyBGZXkiD%2FHjrkU%3D&reserved=0)).

To solve this I created a wrapper subscriber in BCPTTestSuite and calls
another event that is public.

I hope this workaround is acceptable.

#### Work Item(s) <!-- Add the issue number here after the #. The issue
needs to be open and approved. Submitting PRs with no linked issues or
unapproved issues is highly discouraged. -->
Fixes #437

Fixes
[AB#498190](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/498190)

---------

Co-authored-by: Marcus Nordlund <marcus.nordlund@navet.se>
Co-authored-by: Jesper Schulz-Wedde <JesperSchulz@users.noreply.github.com>
Co-authored-by: Chethan Thopaiah <41570277+ChethanT@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved The issue is approved BCIdea Issue related to a BCIdea
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants