-
Notifications
You must be signed in to change notification settings - Fork 15
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
Discover SAP system running on a JAVA stack #2820
Conversation
386fe76
to
0c7a902
Compare
0c7a902
to
21b3186
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpick comments in the frontend.
About the backend, and its test.
Do you think we need so many new tests for such a simple thing?
I mean, I think that simply with one new test would've been enough. Just a test to see that a j2ee instance emits the sap system discovered event. The rest of the scenarios are already well tested
assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.stories.jsx
Outdated
Show resolved
Hide resolved
assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx
Outdated
Show resolved
Hide resolved
a78c9f4
to
f30d6c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @EMaksy
Yet other set of comments about the frontend changes 😝
As a comment, now in Storybook, in SapSystemsOverview
entry, all the types, always are ABAP/JAVA
which is a totally exceptional deployment.
Could we have only ABAP
or JAVA
and the combo of both only in the custom story?
assets/js/pages/SapSystemsOverviewPage/SapSystemsOverview.test.jsx
Outdated
Show resolved
Hide resolved
const sapSystemTypes = [ | ||
'ABAP', | ||
'J2EE', | ||
'ABAP|J2EE', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you are trying to test, but this entry you are putting is not a real scenario.
Here, if you want to test ABAP and JAVA together, you should have one instance with ABAP
features and other instance with J2EE
features. Not 1 instance with both, which doesn't exist
'', | ||
]; | ||
|
||
const sapSystems = sapSystemFactory.buildList(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a factory to modify just after that looks like an anti-pattern.
Why don't you create the value using the factory with the wanted entries?
Something maybe like:
const sapSystems = expectedSapSystemTypes
.map((type) => sapSystemFactory.build({ application_instances : applicationInstanceFactory.buildList(2, { features: type }))
a6e15b8
to
1d69f2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you buddy!
Looks great
1d69f2e
to
eb5ff44
Compare
eb5ff44
to
6a61f6d
Compare
Description
This PR will enable the discovery of a SAP system running on a JAVA stack. A Java sap system is discovered and displayed in the SAP systems overview.
This pr adds a Type field in the Sap systems overview view, so the user can directly see if the discovered system is running on ABAP, JAVA or in a ABAP/JAVA.
Note: This pr contains a lot of almost duplicated back-end tests, this was needed to cover the new check for a sap system.
Previously we checked if a sap system runs on ABAP, now we check for ABAP and/or JAVA.
Preview from Storybook
Did you add the right label?
How was this tested?
Added fronted test
Fixed cypress e2e test (New e2e tests will be added in a second pr as this pr gets a little bit too big)
Updated existing and added new back-end sap systems tests.
Updated storybook
DONE
Did you update the documentation?
No updates needed at this point.