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 Unique Id for all test class instances #3083

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

krmahadevan
Copy link
Member

@krmahadevan krmahadevan commented Mar 5, 2024

Closes #3079

Fixes #3079 .

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt
  • Auto applied styling via ./gradlew autostyleApply

We encourage pull requests that:

  • Add new features to TestNG (or)
  • Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.

Summary by CodeRabbit

  • New Features
    • Introduced unique ID association with test class objects for better tracking and identification.
    • Enhanced DataProvider behavior to align with previous versions for retrying tests.
    • Enabled dynamic adjustment of threads, improving test execution flexibility.
  • Refactor
    • Replaced synchronized blocks with ReentrantLock for improved concurrency handling.
    • Updated internal handling and identification of test instances with IdentifiableObject and UUID.
    • Deprecated and introduced new methods to accommodate changes in instance handling and identification.
  • Bug Fixes
    • Fixed Javadoc formatting issues.
  • Tests
    • Added and updated tests to validate new functionalities and changes in instance management.
  • Documentation
    • Updated documentation to reflect changes in behavior and new features.

@krmahadevan krmahadevan requested a review from juherr as a code owner March 5, 2024 07:15
Copy link

coderabbitai bot commented Mar 5, 2024

Walkthrough

This update introduces a significant shift in how TestNG handles test class objects, focusing on enhancing object management and internal workings. By associating unique IDs with test class instances and adopting IdentifiableObject and IObject for instance handling, TestNG aims to improve memory management and internal mapping. Additionally, adjustments to DataProvider behavior and thread handling align the tool more closely with user expectations and past versions.

Changes

Files Summary
IClass.java, ITestNGMethod.java, ... Introduced unique ID association via UUID and IdentifiableObject, deprecated and replaced certain methods for enhanced instance management.
TestClass.java, BaseClassFinder.java, ... Updated to use IdentifiableObject and IObject for instance handling, improving object management and internal mappings.
ConfigurationMethod.java, FactoryMethod.java, ... Adjusted method signatures and logic to utilize IObject.IdentifiableObject, enhancing compatibility and object handling.
IObject.java, IInstanceIdentity.java Added interfaces for managing associations of classes with instances and providing unique IDs, facilitating better internal object management and mapping.
TestInvoker.java, TestMethodWorker.java, ... Modified object handling to align with the new instance management approach, improving internal operations and memory management.
testng-core/src/test/... Updated tests to reflect and validate the new instance management approach and unique ID association functionality.

Assessment against linked issues

Objective Addressed Explanation
Associate a unique id with every test class object instantiated by TestNG (#3079)

Poem

🐇 Oh, in the land of tests and code,
Where TestNG's lights brightly glowed,
A change was made, with care and thought,
Unique IDs, in objects wrought.

🌟 Through fields of green, the changes hop,
Memory managed, without a stop.
In TestNG's realm, the rabbits cheer,
For better tests, the future's here! 🚀

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between eb80671 and c3d6058.
Files ignored due to path filters (1)
  • testng-core/src/test/resources/testng.xml is excluded by: !**/*.xml
Files selected for processing (26)
  • CHANGES.txt (1 hunks)
  • testng-core-api/src/main/java/org/testng/IClass.java (2 hunks)
  • testng-core-api/src/main/java/org/testng/IObject.java (1 hunks)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (6 hunks)
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11 hunks)
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/ObjectIdTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files not reviewed due to errors (1)
  • (no review received)
Additional comments: 87
testng-core-api/src/main/java/org/testng/IClass.java (4)
  • 7-7: The extension of IClass by IObject aligns with the PR's objective to associate unique IDs with test class instances. This architectural change facilitates the management of test class instances and their unique identification.
  • 30-32: Deprecating getInstances(boolean create) in favor of a new method that includes an error message prefix is a good practice. It enhances error reporting by providing more context when instances cannot be created or retrieved.
  • 35-35: Introducing getInstances(boolean create, String errorMsgPrefix) as a default method ensures backward compatibility while encouraging the use of the new method signature for enhanced error context. This is a thoughtful addition.
  • 40-40: Deprecating addInstance(Object instance) without introducing a new method to replace it directly in this interface might indicate that instance management is being centralized elsewhere, possibly within the IObject interface or its implementations. Ensure that this change aligns with the overall architecture and does not leave a gap in functionality.
Verification successful

The search results confirm that instance management is being centralized in the IObject interface and its implementations, as indicated by the presence of the addObject(IdentifiableObject instance) method. This suggests that the deprecation of addInstance(Object instance) in the IClass interface aligns with an architectural shift towards centralizing instance management, and there does not appear to be a gap in functionality. Therefore, the review comment is consistent with the changes in the codebase.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that instance management is adequately handled elsewhere in the codebase.
rg "addObject\(IdentifiableObject instance\)" --type java

Length of output: 766

testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (2)
  • 5-5: The addition of the org.testng.IObject import is necessary for the changes made in this file, specifically for using IObject.IdentifiableObject in method signatures. This change is consistent with the PR's objectives.
  • 36-36: Changing the instance parameter type in the findOrCreateIClass method from Object to IObject.IdentifiableObject is a key part of implementing the unique ID feature for test class instances. This ensures that every instance managed by BaseClassFinder is identifiable, supporting the overall goal of improved instance management and garbage collection.
testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (2)
  • 3-3: The addition of the org.testng.IObject import is necessary for the changes made in this file, specifically for using IObject.IdentifiableObject in the test method. This change is consistent with the PR's objectives.
  • 40-40: Modifying the argument passed to IdentifiableObject in the findDependedUponMethods method to use an instance of the test class aligns with the goal of associating unique IDs with test class instances. This ensures that the test infrastructure itself is compatible with the new unique ID feature.
testng-core/src/test/java/test/factory/ObjectIdTest.java (2)
  • 18-28: The test ensureOnlyOneObjectIdExistsForNormalTestClass correctly verifies that a unique ID is associated with each instance of SampleTestCase. This test ensures that the implementation of unique IDs works as expected for standard test classes.
  • 30-40: The test ensureOnlyOneObjectIdExistsForFactoryPoweredTestClass verifies that a unique ID is associated with each instance created by a factory, with the expected number of unique IDs matching the number of instances created. This test ensures that the implementation of unique IDs works as expected for factory-powered test classes.
testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1)
  • 17-66: The SampleTestCase class correctly implements the recording of instances and their unique IDs in a static map. This setup is crucial for verifying that unique IDs are properly associated with each test class instance. The use of ConcurrentHashMap for objectMap ensures thread safety, which is important given the parallel execution of tests.
testng-core-api/src/main/java/org/testng/IObject.java (1)
  • 7-64: The introduction of the IObject interface and the IdentifiableObject nested class is a key part of the PR's objective to associate unique IDs with test class instances. The design allows for the encapsulation of test class instances with their unique IDs, facilitating better instance management and garbage collection. The implementation of equals and hashCode methods in IdentifiableObject based on the instance ensures correct behavior in collections.
testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1)
  • 19-65: The FactoryTestCase class correctly implements a factory method and the recording of instances and their unique IDs in a static map. This setup is crucial for verifying that unique IDs are properly associated with each factory-powered test class instance. The use of ConcurrentHashMap for objectMap ensures thread safety, which is important given the parallel execution of tests and the potential for concurrent modifications to the map.
testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1)
  • 64-64: Replacing the lambda expression with an instance of IdentifiableObject initialized with the lambda expression in GuiceHelperTest ensures that the unique ID feature is integrated with Guice injection. This change demonstrates the adaptability of the unique ID feature to different parts of the TestNG framework, including dependency injection.
testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (2)
  • 100-108: The addition of methods to handle IdentifiableObject instances in FakeTestClass ensures that the unique ID feature can be tested in a controlled environment. These methods are crucial for verifying the behavior of the unique ID feature in various scenarios.
  • 118-119: The addObject(IdentifiableObject instance) method in FakeTestClass allows for the addition of identifiable instances to the test class, supporting the testing of instance management with unique IDs. This method complements the getObjects methods by providing a way to populate the test class with instances for testing.
testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (3)
  • 25-25: Changing the type of m_instances to IdentifiableObject[] in NoOpTestClass is a necessary update to align with the unique ID feature. This change ensures that even no-operation test classes can handle identifiable instances, maintaining consistency across the framework.
  • 137-140: The introduction of the getObjects(boolean create) method returning IdentifiableObject[] ensures that NoOpTestClass can provide identifiable instances, aligning with the unique ID feature's requirements. This method supports the consistent handling of instances across different parts of the TestNG framework.
  • 155-156: The addObject(IdentifiableObject instance) method allows for the addition of identifiable instances to NoOpTestClass, supporting the framework's ability to manage instances with unique IDs consistently, even in no-operation contexts.
testng-core/src/main/java/org/testng/internal/ClassImpl.java (10)
  • 3-3: Importing java.util.Arrays is a good addition given the use of Arrays.stream in the getInstances method to work with the new IdentifiableObject type.
  • 27-27: Changing the type of m_defaultInstance from Object to IdentifiableObject aligns with the PR's objective to enhance object management within TestNG by associating unique IDs with test class objects.
  • 29-29: Renaming m_instances to identifiableObjects and changing its type to a list of IdentifiableObject is a logical step following the introduction of IdentifiableObject. This change improves clarity and aligns with the new approach.
  • 32-32: The change in the type of m_instance to IdentifiableObject is consistent with the overall goal of the PR. It ensures that each test class instance is wrapped with an object that includes a unique ID.
  • 53-54: The use of IdentifiableObject.unwrap(instance) to check if the instance is of type ITest is a good practice. It ensures that the unique ID functionality is transparent to existing logic that expects plain Object instances.
  • 104-115: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [94-110]

The getDefaultInstance method's adaptation to work with IdentifiableObject is well-implemented. Wrapping the raw object with IdentifiableObject when creating a default instance ensures consistency with the new object management approach.

  • 123-126: Converting IdentifiableObject instances back to Object in getInstances using Arrays.stream and map is a neat way to maintain backward compatibility with existing code that expects Object[].
  • 129-131: The addObject method correctly adds an IdentifiableObject instance to the identifiableObjects list, ensuring that all test class instances are managed with their unique IDs.
  • 133-149: The getObjects method's logic to return IdentifiableObject[] and update m_instanceHashCodes based on the instances' hash codes is correctly implemented. It ensures that the unique ID functionality is integrated seamlessly with existing features that rely on instance hash codes.
  • 161-161: The addInstance method's adaptation to wrap the provided Object instance with IdentifiableObject before adding it to the list is a necessary change to align with the new object management strategy.
testng-core/src/main/java/org/testng/internal/TestNGMethod.java (4)
  • 9-9: The import of org.testng.IObject is necessary for the file to use IObject.IdentifiableObject, aligning with the PR's objective to manage test class instances with unique IDs.
  • 37-37: The constructor of TestNGMethod now correctly expects an IObject.IdentifiableObject instead of a plain Object. This change is crucial for integrating the unique ID functionality into the method handling logic.
  • 47-47: The private constructor's adaptation to accept IObject.IdentifiableObject is consistent with the changes made to the public constructor, ensuring that all instances handled by TestNGMethod are associated with unique IDs.
  • 175-175: The clone method's modification to create a new IdentifiableObject with the instance and its ID ensures that the cloned TestNGMethod retains the unique ID association of the original instance. This is a critical aspect of maintaining the integrity of the unique ID functionality across method clones.
testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1)
  • 181-181: The modification in createListenerFactory to access the instance via ic.getObjects(false)[0].getInstance() is necessary to adapt to the use of IdentifiableObject. This ensures that the listener factory is instantiated with the correct object instance, aligning with the new object management strategy.
testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2)
  • 7-7: The import of org.testng.IObject is necessary for the file to use IObject.IdentifiableObject, aligning with the PR's objective to manage test class instances with unique IDs.
  • 191-195: The adaptation in computeParameters to use IObject.IdentifiableObject and access the instance via getInstance() is a necessary change to align with the new object management strategy. This ensures that the correct instance is used when instantiating nested classes.
testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2)
  • 6-6: The import of java.util.UUID is correctly added to support the generation of unique IDs for test instances. This aligns with the PR's objective to enhance instance management within TestNG.
  • 368-371: The implementation of getInstanceId() method in WrappedTestNGMethod correctly delegates the call to the wrapped testNGMethod instance. This ensures that the unique ID functionality is properly propagated through the proxy pattern. However, it's important to ensure that the testNGMethod instance itself supports this new method and returns a meaningful UUID.
Verification successful

Given the lack of output from the initial script, it's necessary to refine our approach to verify the presence of the getInstanceId method in classes implementing ITestNGMethod. Let's start by identifying any classes that implement ITestNGMethod and then check for the getInstanceId method within those classes.


Given the repeated lack of output from our attempts to directly find classes implementing ITestNGMethod and those with a getInstanceId method, it's necessary to verify the existence of the ITestNGMethod interface itself within the codebase. This step will help confirm that we are searching within the correct context and that the interface is indeed part of the codebase.


Given the confirmed existence of the ITestNGMethod interface within the codebase, the next step is to broaden our search approach to identify any potential usage or implementation of the getInstanceId method. This will help in understanding how and where this method is supported across the codebase.


The search across the codebase has confirmed the presence of the getInstanceId method in both the ITestNGMethod interface and its various implementations, including TestNGMethod.java, ConfigurationMethod.java, BaseTestMethod.java, and others. This supports the review comment's assertion that the getInstanceId method is correctly delegated to the testNGMethod instance in WrappedTestNGMethod, ensuring that the unique ID functionality is properly propagated through the proxy pattern. Based on these findings, the implementation of getInstanceId() in WrappedTestNGMethod is verified to be in alignment with the expected behavior.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the testNGMethod instance (likely implementations of ITestNGMethod) supports getInstanceId method and returns a valid UUID.
ast-grep --lang java --pattern $'class $_ implements ITestNGMethod { $$$ getInstanceId() { $$$ } $$$ }'

Length of output: 104



Script:

#!/bin/bash
# First, find any classes implementing ITestNGMethod
ast-grep --lang java --pattern $'class $_ implements ITestNGMethod { $$$ }'

Length of output: 76



Script:

#!/bin/bash
# Verify the existence of ITestNGMethod interface in the codebase
rg "interface ITestNGMethod" -l

Length of output: 93



Script:

#!/bin/bash
# Search for the getInstanceId method across the entire codebase
rg "getInstanceId" -l

Length of output: 623

testng-core-api/src/main/java/org/testng/ITestNGMethod.java (2)
  • 6-6: The import of java.util.UUID is correctly added to support the new getInstanceId() method, which is essential for generating unique IDs for test instances.
  • 301-307: The addition of the getInstanceId() method in the ITestNGMethod interface is a crucial part of the PR's objective to associate unique IDs with test class objects. The default implementation returning null is a safe placeholder, but concrete implementations must override this method to return a meaningful UUID. It's important to ensure all implementations of ITestNGMethod provide a valid override of this method.
testng-core/src/main/java/org/testng/internal/FactoryMethod.java (4)
  • 15-15: The import of org.testng.IObject is correctly added to support the use of IObject.IdentifiableObject within FactoryMethod. This aligns with the PR's objective to enhance instance management by associating unique IDs with test class objects.
  • 75-80: The modification of the FactoryMethod constructor to accept an IObject.IdentifiableObject instead of a plain Object instance is a key part of integrating the unique ID functionality. This change ensures that factory methods can work with identifiable test instances, supporting the overall goal of improved instance management.
  • 82-82: The use of IObject.IdentifiableObject.unwrap(identifiable) to extract the actual instance from the IdentifiableObject wrapper is correct and necessary for backward compatibility with existing code that expects plain Object instances. This approach maintains the integrity of the unique ID feature while ensuring minimal disruption to existing functionalities.
  • 122-122: The assignment of m_instance using getInstance() is a subtle yet important change. It ensures that the instance variable within FactoryMethod correctly references the unwrapped test instance. This change is crucial for maintaining the expected behavior of factory methods while incorporating the new unique ID feature.
testng-core/src/main/java/org/testng/TestClass.java (6)
  • 109-115: The update to use IdentifiableObject instances and the subsequent stream operations to handle test instances are correctly implemented. This change is crucial for integrating the unique ID functionality into the TestClass and ensuring that test names can be derived from ITest instances when available. This approach enhances the flexibility and robustness of test instance management.
  • 126-129: The addition of the getObjects method override in TestClass to return an array of IdentifiableObject is a necessary update to support the new unique ID feature. This method ensures that all parts of the TestNG framework can work with identifiable test instances, facilitating better instance management and garbage collection.
  • 136-139: Similar to the previous comment, the addition of the getObjects method override with an errorMsgPrefix parameter is correctly implemented. This ensures consistency in how identifiable test instances are retrieved throughout the TestNG framework, aligning with the PR's objectives.
  • 151-154: The implementation of the addObject method in TestClass to accept an IdentifiableObject instance is a key part of integrating the unique ID functionality. This change allows for the addition of identifiable test instances to the internal structures of TestNG, supporting the overall goal of improved instance management.
  • 148-163: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [160-199]

The modifications to use IdentifiableObject instances in the initialization of configuration methods are correctly implemented. These changes ensure that all configuration methods can work with identifiable test instances, which is essential for the unique ID feature to function as intended across the TestNG framework.

  • 251-251: The update to iterate over IdentifiableObject instances when creating test methods is correctly implemented. This ensures that test methods are associated with identifiable test instances, aligning with the PR's objective to improve test instance management through the use of unique IDs.
testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (2)
  • 10-10: The addition of the org.testng.IObject import is necessary for the new functionality introduced in this PR. It enables the use of IObject.IdentifiableObject within the test methods.
  • 237-237: The modification to wrap the object with new IObject.IdentifiableObject(object) is essential for associating unique IDs with test class objects. This change aligns with the PR's objective to improve internal handling and garbage collection of test class instances. Ensure that this change does not inadvertently affect the functionality of existing tests.
testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (3)
  • 9-11: The addition of imports for java.util.Objects and java.util.UUID supports the new functionality of generating unique keys for locking mechanisms. These are necessary for the enhancements made in this PR.
  • 52-52: The introduction of a static final field lock of type KeyAwareAutoCloseableLock is crucial for the new locking mechanism based on unique keys. Ensure that this locking mechanism is thread-safe and does not introduce deadlocks.
  • 126-128: The implementation of the locking mechanism using UUID for generating unique keys and KeyAwareAutoCloseableLock for managing locks is well-designed. It ensures that each test method instance is handled uniquely, which is crucial for the parallel execution of tests. Using the try-with-resources statement for automatic lock release is a good practice. However, it's important to thoroughly test this implementation to identify any potential issues related to locking, such as deadlocks or performance impacts.

Also applies to: 137-137

testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (11)
  • 17-17: The import of org.testng.IObject is correctly added to support the new functionality of associating unique IDs with test class objects.
  • 38-38: The change from Object to IObject.IdentifiableObject in the declaration of m_instanceMap is crucial for associating unique IDs with test class objects. This aligns with the PR's objectives and enhances the handling of test class instances.
  • 51-51: The method signature of TestNGClassFinder constructor now accepts Map<Class<?>, List<IObject.IdentifiableObject>> instead of a map with plain Object instances. This change is necessary for the integration of unique IDs with test class objects and ensures type safety.
  • 75-80: The loop iterating over m_instanceMap and calling ic.addObject(instance) for each IdentifiableObject ensures that all test class instances are correctly associated with their unique IDs. This is a key part of the implementation for managing test class instances more effectively.
  • 88-88: The method signature of processClass has been correctly updated to accept Map<Class<?>, List<IObject.IdentifiableObject>> as an argument. This change is consistent with the overall goal of associating unique IDs with test class objects.
  • 100-101: The retrieval and handling of IdentifiableObject instances from instanceMap within processClass method are correctly implemented. This ensures that each test class instance is managed with its unique ID.
  • 169-171: The method processFactory correctly retrieves IdentifiableObject instances using ic.getObjects(false). This is part of the mechanism to ensure that factory methods work with the new system of unique IDs for test class instances.
  • 184-196: The loop in processFactory method that handles instances returned by factory methods is correctly updated to work with IdentifiableObject and IInstanceInfo. This ensures that both types of objects are properly managed with their unique IDs.
  • 330-330: The method addInstance(IInstanceInfo<T> ii) is correctly updated to wrap the instance with IObject.IdentifiableObject. This is essential for associating unique IDs with instances that are added to the test context.
  • 333-338: The method addInstance(IObject.IdentifiableObject o) correctly determines the class of the instance, taking into account whether it is an instance of IParameterInfo. This ensures that the correct class is used when adding instances with unique IDs.
  • 343-345: The method addInstance(Class<S> clazz, IObject.IdentifiableObject instance) correctly adds IdentifiableObject instances to m_instanceMap. This is a key part of managing test class instances with unique IDs.
testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11)
  • 8-9: The addition of import statements for Optional and IObject aligns with the changes made in the rest of the file to support the new functionality of associating unique IDs with test class instances. This is a necessary step to ensure that the new types are available for use within the file.
  • 71-71: Changing the parameter type from Object to IObject.IdentifiableObject in the constructor signature is a key part of implementing the unique ID functionality. This change ensures that each test class instance can be associated with a unique ID, facilitating better internal handling and potentially improving garbage collection efficiency.
  • 126-126: The change in the parameter type for another constructor of ConfigurationMethod is consistent with the overall goal of associating unique IDs with test class instances. This ensures that all instances created or manipulated by this class are properly wrapped with their unique IDs.
  • 160-160: The modification in the createMethods static method to accept IObject.IdentifiableObject instead of Object is crucial for ensuring that all test methods created by this method are associated with their unique IDs. This change is consistent with the PR's objective and is necessary for the correct functioning of the new feature.
  • 200-200: The change in the parameter type for createSuiteConfigurationMethods is aligned with the changes in other parts of the file and is necessary for the implementation of the unique ID functionality. This ensures that suite configuration methods are also associated with unique IDs.
  • 224-224: Similarly, the change in the parameter type for createTestConfigurationMethods supports the PR's objective by ensuring that test configuration methods are associated with unique IDs. This is consistent with the changes made in other parts of the file.
  • 247-247: The modification in the parameter type for createClassConfigurationMethods is necessary for the implementation of the unique ID functionality, ensuring that class configuration methods are associated with unique IDs. This change is consistent with the overall goal of the PR.
  • 269-269: The change in the parameter type for createBeforeConfigurationMethods aligns with the PR's objective by ensuring that before configuration methods are associated with unique IDs. This is consistent with the changes made in other parts of the file.
  • 300-300: The modification in the parameter type for createAfterConfigurationMethods supports the PR's objective by ensuring that after configuration methods are associated with unique IDs. This is consistent with the changes made in other parts of the file.
  • 331-331: The change in the parameter type for createTestMethodConfigurationMethods is aligned with the changes in other parts of the file and is necessary for the implementation of the unique ID functionality. This ensures that test method configuration methods are also associated with unique IDs.
  • 511-511: The cloning method has been updated to ensure that the cloned ConfigurationMethod instance is associated with the same unique ID as the original instance. This is a crucial change for maintaining the integrity of the unique ID functionality across cloned instances.
testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (8)
  • 12-12: The import of java.util.UUID is correctly added to support the generation or handling of unique IDs within the class.
  • 19-19: The import of org.testng.IObject is necessary for the changes made in this file, specifically for using the IObject.IdentifiableObject type.
  • 86-86: The addition of the m_instance field of type IObject.IdentifiableObject is crucial for associating a unique ID with each test class instance. This change aligns with the PR's objective to improve garbage collection efficiency by making test class instances more identifiable.
  • 96-96: The constructor now accepts an IObject.IdentifiableObject instance as a parameter. This modification is necessary to initialize the m_instance field with a unique ID wrapped test class instance. It's important to ensure that all invocations of this constructor throughout the codebase are updated to pass the required IdentifiableObject.
  • 153-157: The getInstance() method has been updated to return the actual test class instance from the IdentifiableObject. This change is necessary to maintain compatibility with existing code that expects an instance of the test class. The use of Optional and method chaining ensures null safety, which is a good practice.
  • 159-161: The addition of the getInstanceId() method is a direct consequence of the PR's objective to associate unique IDs with test class instances. This method allows for the retrieval of the unique ID associated with the test class instance, which is essential for the improved garbage collection mechanism.
  • 392-393: The modification to the hashCode() method to include the identity hash code of the instance obtained via getInstance() is a thoughtful addition. It ensures that the hash code considers the unique identity of the test class instance, which is crucial for using these instances as keys in maps or collections.
  • 803-807: The getFactoryMethodParamsInfo() method now checks if the instance wrapped by IdentifiableObject implements IParameterInfo. This change is necessary to support scenarios where test class instances are created with parameters, and it's important to retrieve these parameters for further processing or reporting.
testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2)
  • 30-30: The addition of the IObject import is necessary for implementing the unique ID functionality for test class instances. This change aligns with the PR's objectives.
  • 385-392: The modification to use IObject.IdentifiableObject for handling test class instances and accessing the actual instance with instance.getInstance() is a key part of integrating the unique ID functionality. This change ensures that the unique ID is properly associated with each test class instance, which is crucial for the objectives of this PR. However, it's important to ensure that all parts of the code that interact with test class instances are updated to accommodate this new approach to avoid any potential issues.
  • Ensure that the rest of the codebase that interacts with test class instances is aware of this change and correctly handles IdentifiableObject instances.
  • Consider the performance implications of adding an additional layer of abstraction with IdentifiableObject. While it's unlikely to have a significant impact, it's worth keeping in mind, especially in large test suites.
Verification successful

The modification to use IObject.IdentifiableObject for handling test class instances, as seen in TestInvoker.java and also applied in NoOpTestClass.java, indicates a consistent application of this approach across the codebase. This supports the integration of the unique ID functionality and suggests that relevant parts of the system have been updated to accommodate this change. It remains important to ensure that all interactions with test class instances are correctly handled and to consider the performance implications of these changes, especially in large test suites.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that all interactions with test class instances have been updated.
rg "getInstances\(true\)" --type java
# Check for potential performance implications in large test suites.
echo "Manual review recommended to assess performance impact in large test suites."

Length of output: 202



Script:

#!/bin/bash
# Corrected verification to search for the actual method usage "getObjects(true)".
rg "getObjects\(true\)" --type java
# Reiterate the recommendation for a manual review to assess performance impact in large test suites.
echo "Manual review recommended to assess performance impact in large test suites."

Length of output: 456

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between eb80671 and 8fdd106.
Files ignored due to path filters (1)
  • testng-core/src/test/resources/testng.xml is excluded by: !**/*.xml
Files selected for processing (26)
  • CHANGES.txt (1 hunks)
  • testng-core-api/src/main/java/org/testng/IClass.java (2 hunks)
  • testng-core-api/src/main/java/org/testng/IObject.java (1 hunks)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (6 hunks)
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11 hunks)
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/ObjectIdTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (25)
  • CHANGES.txt
  • testng-core-api/src/main/java/org/testng/IClass.java
  • testng-core-api/src/main/java/org/testng/IObject.java
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/TestClass.java
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java
  • testng-core/src/test/java/test/factory/ObjectIdTest.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java
Additional comments: 2
testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2)
  • 6-6: The addition of java.util.UUID import is appropriate for generating unique IDs, aligning with the PR's objective to associate unique IDs with test class objects. This import is necessary for the implementation of the getInstanceId() method.
  • 3-9: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]

The use of java.util.Random for generating a multiplicationFactor is noted. While this usage is not directly related to security contexts such as password or token generation, it's important to be aware of the limitations of Random for cryptographic purposes. In this specific case, the use of Random is acceptable as it's used to generate a hashcode multiplier, not for security-sensitive operations.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 832cff4 and e174d14.
Files ignored due to path filters (1)
  • testng-core/src/test/resources/testng.xml is excluded by: !**/*.xml
Files selected for processing (26)
  • CHANGES.txt (1 hunks)
  • testng-core-api/src/main/java/org/testng/IClass.java (2 hunks)
  • testng-core-api/src/main/java/org/testng/IObject.java (1 hunks)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (6 hunks)
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11 hunks)
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/ObjectIdTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (25)
  • CHANGES.txt
  • testng-core-api/src/main/java/org/testng/IClass.java
  • testng-core-api/src/main/java/org/testng/IObject.java
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/TestClass.java
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java
  • testng-core/src/test/java/test/factory/ObjectIdTest.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java
Additional comments: 3
testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (3)
  • 6-6: The import of UUID is correctly added to support the generation of unique IDs for test instances. This aligns with the PR's objective to enhance instance management within TestNG.
  • 368-371: The implementation of getInstanceId() correctly delegates the call to the wrapped ITestNGMethod instance. This approach is consistent with the proxy pattern used in WrappedTestNGMethod. Ensure that the ITestNGMethod interface and its implementations are updated to include the getInstanceId() method, as this change impacts the contract of ITestNGMethod.
  • 3-9: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [22-22]

The use of Random to generate a multiplicationFactor for the hashCode() method is noted. While the static analysis tool suggests using SecureRandom for cryptographic strength, the current usage context does not involve security-sensitive operations. However, it's important to ensure that the randomness provided by Random is sufficient for the intended purpose of creating a unique hashCode. If the uniqueness of hashCode is critical, consider evaluating the distribution and collision probability of the generated values.

Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

LGTM except I think it is details of implementation that should be exposed into the API.

Except the deprecations, could you try to remove the API update?

testng-core-api/src/main/java/org/testng/IObject.java Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 438674c and 28fedc0.
Files ignored due to path filters (1)
  • testng-core/src/test/resources/testng.xml is excluded by: !**/*.xml
Files selected for processing (26)
  • CHANGES.txt (1 hunks)
  • testng-core-api/src/main/java/org/testng/IClass.java (1 hunks)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (3 hunks)
  • testng-core-api/src/main/java/org/testng/internal/IObject.java (1 hunks)
  • testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (6 hunks)
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11 hunks)
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (7 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/ObjectIdTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (24)
  • CHANGES.txt
  • testng-core-api/src/main/java/org/testng/IClass.java
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/TestClass.java
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java
  • testng-core/src/test/java/test/factory/ObjectIdTest.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java
Additional comments: 10
testng-core-api/src/main/java/org/testng/internal/IObject.java (8)
  • 22-22: The method getObjects is well-defined, providing a mechanism to retrieve instances associated with a class, optionally creating them if necessary. This method supports the core functionality of managing test instances with unique IDs.
  • 25-25: The method getInstanceHashCodes offers a way to retrieve hash codes of instances, which could be useful for debugging or logging purposes. However, ensure that the use of hash codes aligns with the unique ID mechanism and does not introduce any potential for confusion or collision.
  • 28-28: The addObject method allows adding a new instance to the list of managed instances. This is crucial for dynamically managing instances, especially in scenarios involving factories or dynamically generated tests.
  • 34-36: The static method instanceHashCodes provides a utility to retrieve hash codes for instances that are compatible with IObject. This method enhances the interface's usability by allowing operations on generic objects without explicit casting by the caller.
  • 44-46: The overloaded static method objects without the errorMsgPrefix parameter simplifies the retrieval of IdentifiableObject arrays by providing a default error message prefix. This method enhances usability by reducing the need for callers to specify common parameters.
  • 56-60: The static method objects with the errorMsgPrefix parameter is a comprehensive way to retrieve IdentifiableObject arrays, allowing for customized error handling. This method is essential for scenarios where detailed error messages are necessary for debugging or logging.
  • 67-72: The static method cast provides a safe way to cast objects to IObject if they implement the interface. This utility method is crucial for working with objects in a type-safe manner, reducing the risk of ClassCastException.
  • 75-115: The nested class IdentifiableObject is well-designed, encapsulating a test class instance and its associated unique ID. The constructors, accessors, and overridden equals and hashCode methods are correctly implemented, ensuring that instances are uniquely identifiable and can be used effectively in collections. This class is central to the enhancement introduced by this pull request, enabling efficient management and identification of test class instances.
testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2)
  • 6-6: The import of java.util.UUID is necessary for the new functionality introduced by the getInstanceId method. This addition is correctly placed and follows Java import conventions.
  • 368-371: The implementation of the getInstanceId method correctly delegates the call to the wrapped ITestNGMethod instance. This approach maintains consistency with the proxy pattern used throughout the WrappedTestNGMethod class. However, ensure that the ITestNGMethod interface and its implementations are updated to include the getInstanceId() method, as this change impacts the contract of ITestNGMethod.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 438674c and a3ec3a3.
Files ignored due to path filters (1)
  • testng-core/src/test/resources/testng.xml is excluded by: !**/*.xml
Files selected for processing (26)
  • CHANGES.txt (1 hunks)
  • testng-core-api/src/main/java/org/testng/IClass.java (1 hunks)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (6 hunks)
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java (8 hunks)
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (11 hunks)
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/IObject.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (5 hunks)
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (7 hunks)
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java (1 hunks)
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java (2 hunks)
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/ObjectIdTest.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (24)
  • CHANGES.txt
  • testng-core-api/src/main/java/org/testng/IClass.java
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/TestClass.java
  • testng-core/src/main/java/org/testng/internal/BaseClassFinder.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/ClassImpl.java
  • testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java
  • testng-core/src/main/java/org/testng/internal/FactoryMethod.java
  • testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
  • testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
  • testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java
  • testng-core/src/main/java/org/testng/internal/TestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/main/java/org/testng/internal/objects/SimpleObjectDispenser.java
  • testng-core/src/test/java/org/testng/internal/DynamicGraphHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodHelperTest.java
  • testng-core/src/test/java/org/testng/internal/MethodInstanceTest.java
  • testng-core/src/test/java/org/testng/internal/dynamicgraph/FakeTestClass.java
  • testng-core/src/test/java/org/testng/internal/objects/GuiceHelperTest.java
  • testng-core/src/test/java/test/factory/ObjectIdTest.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java

@krmahadevan krmahadevan requested a review from juherr March 8, 2024 08:07
@krmahadevan
Copy link
Member Author

@juherr - I think that if we get this PR merged, maybe I can take a jab at #1621
because this PR now provides for a way to defer the object instantiation to as late as possible.
When you get time, please help take a look at this PR

Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

That's a better solution but I think it need another iteration.

@krmahadevan krmahadevan requested a review from juherr March 14, 2024 03:52
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a3ec3a3 and 3e05a87.
Files selected for processing (7)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (7 hunks)
  • testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (6)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a3ec3a3 and afc7b6c.
Files selected for processing (7)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (7 hunks)
  • testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (7)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a3ec3a3 and 5747e77.
Files selected for processing (7)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java (7 hunks)
  • testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java (1 hunks)
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java (3 hunks)
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (4 hunks)
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java (1 hunks)
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java (1 hunks)
Files skipped from review as they are similar to previous changes (7)
  • testng-core-api/src/main/java/org/testng/ITestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
  • testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java
  • testng-core/src/main/java/org/testng/internal/WrappedTestNGMethod.java
  • testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
  • testng-core/src/test/java/test/factory/issue3079/FactoryTestCase.java
  • testng-core/src/test/java/test/factory/issue3079/SampleTestCase.java

@krmahadevan krmahadevan merged commit 3308b3c into testng-team:master Mar 14, 2024
4 of 9 checks passed
@krmahadevan krmahadevan deleted the fix_3079 branch March 14, 2024 05:01
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.

Associate a unique id with every test class object instantiated by TestNG
3 participants