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

Added TSQ manager and its IVT program #984

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Anuprakash-Moothedath
Copy link

Added TSQ manager and its IVT under galasa-managers-cicsts-parent folder. TSQ manager implements the interface ITsqHandler with following methods:

  1. setQName() - to set the TSQ name
  2. getQName() - to get the TSQ name
  3. readQ() - to read the TSQ with name set using setQName() method
  4. writeQ() - to write to TSQ with name set using setQName() method
  5. deleteQ() - to delete the TSQ with name set using setQName() method
  6. makeRecoverable() - to make the TSQ recoverable

The ITsqHandler instance can be created using the following code snippet:

@CicsRegion()
public ICicsRegion cicsRegion;
...
ITsqHandler tsq = cicsRegion.tsq();

The documentation on how to use TSQ manager is present in - galasa-managers-parent/galasa-managers-cicsts-parent/dev.galasa.cicsts.tsq.manager/tsq_manager_codesnippet.md.

The IVT for TSQ manager is present in - galasa-managers-parent/galasa-managers-cicsts-parent/dev.galasa.cicsts.tsq.manager.ivt.

Signed-off-by: Anuprakash Moothedath <anuprakash.moothedath@ibm.com>
@galasa-team
Copy link
Contributor

Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building'

@hobbit1983 hobbit1983 self-requested a review August 28, 2024 12:13
@hobbit1983
Copy link
Member

@techcobweb @Anuprakash-Moothedath adding myself as a reviewer

@techcobweb
Copy link
Contributor

techcobweb commented Aug 28, 2024

On a more general note, I would like to see some unit tests added to make sure that this code does what you think it should be doing in the future. That will help with maintaining it.
Ideally without using mockito/powermock, but just using simple interface injection.

You could mock the Ceci manager and make sure that it's passing the right information...etc.

Copy link
Contributor

@techcobweb techcobweb left a comment

Choose a reason for hiding this comment

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

some initial comments from me.

Copy link
Member

@hobbit1983 hobbit1983 left a comment

Choose a reason for hiding this comment

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

Love the contribution, but I think we need to make some changes here to stick to the conventions, which I agree, are not really codified anywhere

  1. You are using the convention of allowing for TSQProviders to change the implementation of the TSQ interactions in a way similar to the CEMT and CEDA managers. However since there is only the 1 way to interact with a TSQ (through CECI) I don't think this is particularly necessary.
  2. Providing the TSQHelper object (which is a bit of a factory object) is a level of abstraction that is a bit confusing as it is representing a set of TSQ on a CICS region. Calling setName and then write or read is confusing, which TSQ does this object currently represent
  3. Would it be clearer to support APIs such as writeQueue("queueName","data") within either the factory or directly within the CICSRegion object?

What is the use case for needing to set recoverable? I feel this would be clearer in the test creating a TSModel as this has an impact across the CICS region

Comment on lines 21 to 27
ext.projectName=project.name
ext.includeInOBR = true
ext.includeInMVP = true
ext.includeInBOM = false
ext.includeInIsolated = true
ext.includeInCodeCoverage = false
ext.includeInJavadoc = false
Copy link
Member

Choose a reason for hiding this comment

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

@techcobweb can you review if these are correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • BOM should be true if you want it included by tests.
  • I don't see why you wouldn't want it in javadoc
  • I don't see why you wouldn't want it to participate in code coverage analysis.

Choose a reason for hiding this comment

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

Hi @techcobweb , This was copied from build.gradle in dev.galasa.cicsts.ceda.manager.ivt. Also I was referring to other build.gradle files under IVTs for CEMT manger and CECI manager. There also I can find similar entries. Could you please advise if I need to follow that or set all these options as true?

@Mark-J-Lawrence
Copy link
Contributor

On a more general note, I would like to see some unit tests added to make sure that this code does what you think it should be doing in the future. That will help with maintaining it. Ideally without using mockito/powermock, but just using simple interface injection.

You could mock the Ceci manager and make sure that it's passing the right information...etc.

+1 to this....see the SDV manager tests on how to do this https://github.com/galasa-dev/managers/tree/main/galasa-managers-parent/galasa-managers-testingtools-parent/dev.galasa.sdv.manager/src/test/java/dev/galasa/sdv/internal

Signed-off-by: Anuprakash Moothedath <anuprakash.moothedath@ibm.com>
@galasa-team
Copy link
Contributor

Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building'

Signed-off-by: Anuprakash Moothedath <anuprakash.moothedath@ibm.com>
@galasa-team
Copy link
Contributor

Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building'

@Anuprakash-Moothedath
Copy link
Author

Anuprakash-Moothedath commented Sep 3, 2024

Hi @hobbit1983 , @techcobweb and @Mark-J-Lawrence , I have made the changes to have an ITsqFactory and create ITsq object by injecting the queue name. Please find the documentation of the changes here. Please have a look into the changes and let me know your comments.

ITsqFactory has the following methods:

- public ITsq createQueue(@NotNull String queueName, @NotNull boolean recoverable) throws TsqException;
- public ITsq createQueue(@NotNull String queueName) throws TsqException;   

ITsq has the following methods:

- public boolean exists() throws TsqException;   // Check the existence of a TSQ. 
- public boolean isRecoverable() throws TsqException; // Check if a TSQ is recoverable. 
- public String readQueue(@NotNull int item) throws TsqException;
- public String readQueueNext() throws TsqException;
- public void writeQueue(@NotNull String data) throws TsqException;
- public void deleteQueue() throws TsqException;

Please refer TsqManagerIVT.java to see how this can be used.

@galasa-team
Copy link
Contributor

Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building'

package dev.galasa.cicsts;

import javax.validation.constraints.NotNull;
public interface ITsqFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer ITSQFactory or TSQFactory. 'I' Prefix is optional, but has been used pretty consistently in Galasa, but I'm personally not wedded to it. Given that the implementation is internal to the bundle and non-reachable, I think that should be TSQFactoryImpl or similar ?

Choose a reason for hiding this comment

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

Hi @techcobweb , I can find that all the interfaces have name starting with 'I' and the implementation of ITsqFactory.java is named as TsqFactoryImpl.java. I thought this is the naming convention followed in Galasa. Please advise if I need to change this or keep it similar to the existing file names in Galasa?

* @return ITsq object
* @throws TsqException if there is a problem in creating the ITsq object
*/
public ITsq createQueue(@NotNull String queueName, @NotNull boolean recoverable) throws TsqException;
Copy link
Contributor

@techcobweb techcobweb Sep 3, 2024

Choose a reason for hiding this comment

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

I think we need a ModelFactory where you can create an ICicsModel or something similar.
eg:

interface ICicsModelFactory {
  public IModel createModel(String namePrefix, boolean isRecoverable, boolean isShared...etc.)
  public IModel getExistingModel(String namePrefix);
}

// Im not sure models are mutable, but..
interface IModel {
  public boolean isRecoverable();
  public void setRecoverable(boolean isRecoverable);
  ... similar for shared.
}

Then in this ITsqFactory interface, we can support:

/** For those queues which don't want or need a model */
public ITsq createQueue(@NotNull String queueName);

And we could also add
public ITsq createQueue( ICicsModel model, String queueNameSuffix );

If we did that, then you would get the 'isRecoverable' from the model explicitly, rather than implicitly via name-prefix matching.

Choose a reason for hiding this comment

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

Hi @techcobweb , TSModel is a CICS resource. As we discussed in the meeting last week, I will be adding a new method in CICSResourceManager for creating TSModel. However as advised by you, that will go as a separate change after TSQ manager. TSQ manager is specific for TSQs only.

Copy link
Contributor

@techcobweb techcobweb left a comment

Choose a reason for hiding this comment

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

Some comments on the interfaces.

@techcobweb
Copy link
Contributor

Also, if you want a re-review, can you pls hit the review request button, as it's easy to miss them otherwise, and it won't show up in my PRs-awaiting-review list. Thanks.

Signed-off-by: Anuprakash Moothedath <anuprakash.moothedath@ibm.com>
@galasa-team
Copy link
Contributor

Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building'

ext.includeInBOM = true
ext.includeInIsolated = true
ext.includeInCodeCoverage = false
ext.includeInJavadoc = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be true for javadoc if these APIs are to appear on the javadoc site.

Choose a reason for hiding this comment

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

Hi @techcobweb , Is this required for IVTs?

@Anuprakash-Moothedath Anuprakash-Moothedath marked this pull request as ready for review September 6, 2024 04:43
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.

7 participants