From ee640c38f100fa44aa570343fd735f07c97d8e64 Mon Sep 17 00:00:00 2001 From: ToineSiebelink Date: Mon, 9 Mar 2026 17:12:31 +0000 Subject: [PATCH] Introducing 'skills' for AI Dev Agents - Used multi-agent approach: Different standard files referring back to .agent skill files (AmazonQ still needs to be told for each new session!) - Add Skill (rules) for copyrights (rules extracted by AmazonQ based on samples) - Add Skill (rules) for java quality (extracted from Wiki by Claude Agent) - Add Skill (rules) for test quality (extracted from code & Wiki by Claude Agent) Issue-ID: CPS-3064 Change-Id: I3da68e6348a540260ca98a322f8fc884e64bc0ac Signed-off-by: ToineSiebelink --- .agents/skills/copyright-manager/SKILL.md | 80 ++++++++ .agents/skills/java-quality/SKILL.md | 160 ++++++++++++++++ .agents/skills/test-quality/SKILL.md | 291 ++++++++++++++++++++++++++++++ .amazonq/rules/agent-standards.txt | 1 + .github/copilot-instructions.md | 1 + AGENTS.md | 11 ++ CLAUDE.md | 1 + 7 files changed, 545 insertions(+) create mode 100644 .agents/skills/copyright-manager/SKILL.md create mode 100644 .agents/skills/java-quality/SKILL.md create mode 100644 .agents/skills/test-quality/SKILL.md create mode 100644 .amazonq/rules/agent-standards.txt create mode 100644 .github/copilot-instructions.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/.agents/skills/copyright-manager/SKILL.md b/.agents/skills/copyright-manager/SKILL.md new file mode 100644 index 0000000000..a915d4f383 --- /dev/null +++ b/.agents/skills/copyright-manager/SKILL.md @@ -0,0 +1,80 @@ +--- +name: copyright-manager +description: Rules for updating copyright headers whenever a file is modified. +--- + +# License Header Rules for OpenInfra Foundation Europe Team +## When to Update License Headers +Update the license header **every time you modify a or create a Java or Groovy file**. +## Rules +### 1. **New files** +Add copyright with current year only: +```java +/* + * ============LICENSE_START======================================================= + * Copyright (C) 2026 OpenInfra Foundation Europe. All rights reserved. + * ================================================================================ +``` +### 2. **If file has OpenInfra copyright already** +Update the year range to current year: +```java +/* + * ============LICENSE_START======================================================= + * Copyright (C) 2021-2026 OpenInfra Foundation Europe. All rights reserved. + * ================================================================================ +``` +### 3. **If file has Nordix copyright** +Replace "Nordix" with "OpenInfra" and update year: +```java +// Change this: +Copyright (C) 2021-2024 Nordix Foundation +// To this: +Copyright (C) 2021-2026 OpenInfra Foundation Europe. All rights reserved. +``` +### 4. **If file has OTHER organization copyright** +Add a "Modifications" line below the original: +```java +/* + * ============LICENSE_START======================================================= + * Copyright (C) 2021 Pantheon.tech + * Modifications Copyright (C) 2026 OpenInfra Foundation Europe. All rights reserved. + * ================================================================================ +``` +### 5. **If modifications line exists** +Update the year range: +```java +// Change this: +Modifications Copyright (C) 2022-2024 OpenInfra Foundation Europe. All rights reserved. +// To this: +Modifications Copyright (C) 2022-2026 OpenInfra Foundation Europe. All rights reserved. +``` +## Standard Template +```java +/* + * ============LICENSE_START======================================================= + * Copyright (C) YYYY[-YYYY] OpenInfra Foundation Europe. All rights reserved. + * ================================================================================ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * ============LICENSE_END========================================================= + */ +``` +## Quick Reference +| Scenario | Action | +|----------|--------| +| **New file** | **Add copyright with current year only** | +| OpenInfra copyright exists | Update end year | +| Nordix copyright exists | Replace with OpenInfra, update year | +| Other org copyright exists | Add "Modifications Copyright (C) YYYY OpenInfra..." | +| Modifications line exists | Update end year | diff --git a/.agents/skills/java-quality/SKILL.md b/.agents/skills/java-quality/SKILL.md new file mode 100644 index 0000000000..e7f25c041d --- /dev/null +++ b/.agents/skills/java-quality/SKILL.md @@ -0,0 +1,160 @@ +# Java Code Quality Skill + +## Overview +This skill ensures Java code follows CPS (Configuration Persistence Service) quality standards and best practices. + +## Core Principle +**Simple is Good, Complex is Bad** - Always prefer simple, readable code over complex patterns. When choosing between solutions, select the one that is easiest to understand and maintain. + +## Code Quality Checks + +### 1. Naming Conventions +- Use consistent naming throughout the codebase +- Don't shorten instance names: + ```java + // Bad: CmHandleState state = new CmHandleState(..); + // Good: CmHandleState cmHandleState = new CmHandleState(..); + ``` +- Variable names should match across method calls: + ```java + // Good: + int orderNumber = 123; + String status = getStatus(orderNumber); + String getStatus(int orderNumber) { ... } + ``` + +### 2. String Constants +- Avoid overuse of constants for simple string literals +- Don't create constants just to avoid duplication - consider extracting a method instead: + ```java + // Bad: + final static String HELLO = "Hello "; + String message = HELLO + name; + + // Good: + String message = "Hello " + name; + // Or better: + String sayHello(final String name) { + return "Hello " + name; + } + ``` + +### 3. Null Safety +- Put constants first in equals() to prevent NPEs: + ```java + // Bad: if (xpath.equals(ROOT_NODE_PATH) {..} + // Good: if (ROOT_NODE_PATH.equals(xpath) {..} + ``` + +### 4. Control Flow +- Avoid using `!` (negation) or `!=` ONLY when an else block is implemented - invert the condition: + ```java + // Bad (has else block): + if (x != null) { + do something; + } else { + report error; + } + + // Good: + if (x == null) { + report error; + } else { + do something; + } + ``` + Note: This rule does NOT apply when there's no else block. Simple guard clauses are acceptable: + ```java + // Acceptable (no else block): + if (x != null) { + throw new Exception("error"); + } + ``` + +- No need for else after return: + ```java + // Bad: + if (ROOT_NODE_PATH.equals(xpath) { + return something; + } else { + return somethingElse; + } + + // Good: + if (x == true) { + return something; + } + return something-else; + ``` + +### 5. Collections +- No need to check isEmpty before iterating: + ```java + // Bad: + if (!myCollection.isEmpty()) { + collection.forEach(some action); + } + + // Good: + collection.forEach(some action); + ``` + +- Initialize collections/arrays with known size: + ```java + // Bad: + void processNames(Collection orginalNames) { + String[] processedNames = new String[0]; + + // Good: + void processNames(Collection orginalNames) { + String[] processedNames = new String[orginalNames.size()]; + ``` + +### 6. Performance +- Use string concatenation instead of String.format (5x slower) where possible: + ```java + // Bad: String.format("cm-handle:%s", cmHandleId); + // Good: "cm-handle:" + cmHandleId; + ``` + +### 7. Spring Annotations +- Use `@Service` when a class includes operations +- Use `@Component` when it is a data object only +- Currently no functional difference but may change in future Spring versions + +### 8. Prefer Simplicity Over Optional Patterns +- Avoid complex Optional chains when simple null checks are clearer: + ```java + // Bad (complex): + Optional optionalResponseBody = + Optional.ofNullable(responseEntity.getBody()) + .filter(Predicate.not(String::isBlank)); + return (optionalResponseBody.isPresent()) ? + convert(optionalResponseBody.get()) : Collections.emptyList(); + + // Good (simple): + String responseBody = responseEntity.getBody(); + if (responseBody == null || responseBody.isBlank()) { + return Collections.emptyList(); + } + return convert(responseBody); + ``` + +## Security Checks + +### Never Log User Data +- Do not log any user data at any log level +- User data may contain sensitive information +- When logging objects, ensure toString() implementation doesn't include user data +- Only log well-defined fields that do not contain user data + +## Commit Guidelines +Follow the [ONAP commit message guidelines](https://wiki.onap.org/display/DW/Commit+Message+Guidelines) + +## Application +When reviewing or writing Java code: +1. Check for these patterns in new/modified code +2. Suggest simpler alternatives when complex code is found +3. Ensure security checks (especially logging) are followed +4. Verify naming conventions are consistent +5. Look for performance anti-patterns (String.format, uninitialized collections) diff --git a/.agents/skills/test-quality/SKILL.md b/.agents/skills/test-quality/SKILL.md new file mode 100644 index 0000000000..4db5cdfa2c --- /dev/null +++ b/.agents/skills/test-quality/SKILL.md @@ -0,0 +1,291 @@ +# Spock/Groovy Testing Guide for CPS-NCMP + +This document describes important aspects of our Spock/Groovy test framework, conventions, and best practices observed in the CPS-NCMP codebase. + +## Framework and Tools + +### Core Testing Framework +- **Spock Framework**: Behavior-driven development (BDD) testing framework for Java and Groovy applications +- **Groovy**: Dynamic JVM language used for writing expressive and readable tests +- **Specification Base Class**: All test classes extend `spock.lang.Specification` + +### Additional Libraries +- **Mockito/Spock Mocks**: For creating mock objects and stubbing behavior +- **Logback**: For testing logging behavior (using `ListAppender`) +- **Hazelcast**: For testing distributed cache functionality +- **Reactor**: For testing reactive/asynchronous operations (e.g., `Mono`, `Flux`) + +## Naming Conventions + +### Test Class Names +- All test classes end with `Spec` suffix +- Example: `DataJobServiceImplSpec`, `TrustLevelManagerSpec`, `NetworkCmProxyFacadeSpec` + +### Test Method Names +- Use descriptive sentences in single quotes +- Follow pattern: `def 'Description of what is being tested'()` +- **Do NOT include expectations in the test name** (e.g., "throws exception", "returns true") +- Use `then:` blocks with descriptions to express expectations instead +- Examples: + - ✅ `def 'Registration with invalid cm handle name.'()` + - ❌ `def 'Registration of invalid cm handle throws exception.'()` + - ✅ `def 'Read data job request.'()` + - ✅ `def 'DMI Registration: Create, Update, Delete & Upgrade operations are processed in the right order'()` + - ✅ `def 'Initial cm handle registration with a cm handle that is not trusted'()` + +### Variable Naming +- Mock objects: Prefix with `mock` (e.g., `mockInventoryPersistence`, `mockDmiSubJobRequestHandler`) +- Object under test: **Always use** `objectUnderTest` (not custom names like `parameterMapper` or class-specific names) +- Test result: **Always use** `result` for the outcome of method calls +- Test data: Use meaningful prefixes like `my` or `some` for values that don't affect the test (e.g., `'my user'`, `'some scope'`) +- Important test values: Use descriptive names without generic prefixes to distinguish them from arbitrary values + +## Test Structure + +### Standard Spock Blocks + +Tests use labeled blocks for clarity and readability: + +```groovy +def 'Test description'() { + given: 'context setup description' + // Setup code + and: 'additional context' + // More setup + when: 'action being tested' + // Execute the method under test + then: 'expected outcome' + // Assertions and verifications + and: 'additional expectations' + // More assertions +} +``` + +### Common Blocks +- **given**: Setup test data and preconditions +- **and**: Additional setup or assertions (improves readability) +- **when**: Execute the code under test +- **then**: Assert expected outcomes and verify mock interactions +- **where**: Define parameterized test data (data-driven tests) +- **expect**: Combined when+then for simple tests +- **setup**: Instance-level setup (runs before each test) +- **cleanup**: Instance-level teardown (runs after each test) + +## Test Patterns + +### 1. Object Under Test Initialization + +Pattern with constructor injection and mocks: +```groovy +class DataJobServiceImplSpec extends Specification { + def mockWriteRequestExaminer = Mock(WriteRequestExaminer) + def mockDmiSubJobRequestHandler = Mock(DmiSubJobRequestHandler) + + def objectUnderTest = new DataJobServiceImpl( + mockDmiSubJobRequestHandler, + mockWriteRequestExaminer, + mockJsonObjectMapper + ) +} +``` + +Alternative with Spy for partial mocking: +```groovy +def objectUnderTest = Spy(new CmHandleRegistrationService( + mockPropertyHandler, + mockInventoryPersistence, + ... +)) +``` + +### 2. Mock Interactions + +**Stubbing return values:** +```groovy +mockInventoryPersistence.getYangModelCmHandle('ch-1') >> new YangModelCmHandle(id: 'ch-1') +``` + +**Verifying method calls:** +```groovy +1 * mockInventoryEventProducer.sendAvcEvent('ch-1', 'trustLevel', 'NONE', 'COMPLETE') +0 * mockInventoryEventProducer.sendAvcEvent(*_) // No calls expected +``` + +**Argument matching:** +```groovy +1 * mockWriteRequestExaminer.splitDmiWriteOperationsFromRequest('my-job-id', dataJobWriteRequest) +mockAlternateIdChecker.getIdsOfCmHandlesWithRejectedAlternateId(*_) >> [] // Any arguments +``` + +### 3. Parameterized Tests (Data-Driven) + +Use `where` block for data tables: +```groovy +def 'Determining cloud event using ce_type header for a #scenario.'() { + given: 'headers contain a header for key: #key' + headers.lastHeader(key) >> header + expect: 'the check for cloud events returns #expectedResult' + assert objectUnderTest.isCloudEvent(headers) == expectedResult + where: 'the following headers (keys) are defined' + scenario | key || expectedResult + 'cloud event' | 'ce_type' || true + 'non-cloud event' | 'other' || false +} +``` + +**Conventions:** +- Use `#variableName` in test name to include parameter values in test output +- Use `|` to separate input columns, `||` before expected output columns +- Variables from `where` block are accessible throughout the test + +### 4. Setup and Cleanup + +```groovy +def setup() { + setupLogger(Level.DEBUG) + mockAlternateIdChecker.getIdsOfCmHandlesWithRejectedAlternateId(*_) >> [] +} + +def cleanup() { + ((Logger) LoggerFactory.getLogger(DataJobServiceImpl.class)).detachAndStopAllAppenders() + hazelcastInstance.shutdown() +} +``` + +### 5. Assertion Patterns + +**Groovy assert (preferred):** +```groovy +assert result.statusCode == HttpStatus.OK +assert trustLevelPerCmHandleId.get('ch-1') == TrustLevel.COMPLETE +``` + +**Spock with block for complex assertions:** +```groovy +with(logger.list.find { it.level == Level.DEBUG }) { + assert it.formattedMessage.contains("Initiating WRITE operation") +} +``` + +**JUnit-style assertions (less common):** +```groovy +assertEquals(expected, actual) +assertTrue(condition) +``` + +### 6. Testing Logging Behavior + +```groovy +def logger = Spy(ListAppender) + +def setup() { + def setupLogger = ((Logger) LoggerFactory.getLogger(DataJobServiceImpl.class)) + setupLogger.setLevel(Level.DEBUG) + setupLogger.addAppender(logger) + logger.start() +} + +def 'Test with logging verification'() { + when: 'method is called' + objectUnderTest.someMethod() + then: 'correct log message is generated' + def loggingEvent = logger.list[0] + assert loggingEvent.level == Level.INFO + assert loggingEvent.formattedMessage.contains('Expected message') +} +``` + +### 7. Testing Reactive Code + +```groovy +def 'Get resource data from DMI (delegation).'() { + given: 'a cm resource address' + def cmResourceAddress = new CmResourceAddress('ncmp-datastore:operational', 'ch-1', 'resource-id') + and: 'get resource data returns a Mono' + mockHandler.executeRequest(cmResourceAddress, 'options', NO_TOPIC, false, 'auth') >> + Mono.just('dmi response') + when: 'get resource data is called' + def response = objectUnderTest.getResourceDataForCmHandle(cmResourceAddress, ...).block() + then: 'response is correct' + assert response == 'dmi response' +} +``` + +## Best Practices + +### 1. Descriptive Test Names (Slogans) +- Use natural language that describes the scenario being tested +- **Do NOT include expectations** - use `then:` blocks with descriptions instead +- Include context about what's being tested +- Examples: + - ✅ `def 'Registration with invalid cm handle name.'()` with `then: 'a validation exception is thrown'` + - ❌ `def 'Registration of invalid cm handle throws exception.'()` + - ✅ `def 'Initial cm handle registration with a cm handle that is not trusted'()` + - ❌ `def 'test1'()` + +### 2. Clear Given-When-Then Structure +- Always use labeled blocks with descriptions +- Descriptions should explain the setup/action/expectation +- **Keep descriptions up to date** with the actual test code +- Use `and` blocks to break up complex setups +- Example: + ```groovy + given: 'a invalid cm handle name' + cmHandle.id = 'invalid,name' + when: 'permission is checked for unauthorized user' + objectUnderTest.checkPermission(someCmh, 'my user', 'some scope') + then: 'exception message contains the user id' + thrown(PermissionException).message.contains('my user') + ``` + +### 3. Mock Setup +- Initialize mocks as instance variables +- Set up default mock behavior in `setup()` when applicable +- Keep test-specific mocking in the test method itself + +### 4. Assertions +- Prefer Groovy's `assert` over JUnit assertions +- Use specific assertions rather than generic ones +- Verify both positive and negative cases + +### 5. Test Data +- **Minimize test data to only what's needed** for the specific test case +- Avoid copying and pasting unnecessary test data from other tests +- Use Groovy Map Constructor to populate only required fields: `new CmHandle(id: 'invalid,name')` +- Use meaningful test data that reflects real scenarios +- Keep test data close to where it's used +- Use constants for commonly reused values (e.g., `NO_TOPIC = null`) +- Clearly distinguish between important values and arbitrary values using prefixes like `my` or `some` + +### 6. Mock Verification +- Verify important interactions explicitly +- Use `0 *` to ensure methods are NOT called when expected +- Use `*_` (any arguments) sparingly; prefer specific argument matching + +### 7. Parameterized Tests +- Use for testing multiple similar scenarios +- Keep data tables readable (align columns) +- Use descriptive scenario names +- Reference parameters in test descriptions with `#paramName` + +## Common Pitfalls to Avoid + +1. **Over-mocking**: Don't mock everything; use real objects for simple data classes +2. **Unclear test names**: Avoid generic names like "test1" or "testMethod" +3. **Expectations in test names**: Don't include "throws exception" or "returns true" in test names - use `then:` blocks instead +4. **Wrong variable names**: Always use `objectUnderTest` and `result`, not custom names +5. **Unnecessary test data**: Minimize test data to only what's needed; don't copy/paste from other tests +6. **Unclear test values**: Use `my` or `some` prefixes for arbitrary values to distinguish from important ones +7. **Outdated descriptions**: Keep `given:`, `when:`, `then:` descriptions synchronized with code +8. **Missing block labels**: Always label blocks (given, when, then) with descriptions +9. **Testing implementation details**: Focus on behavior, not internal implementation +10. **Brittle tests**: Avoid over-specifying mock interactions; verify what matters +11. **Not using `where` blocks**: Don't duplicate similar tests; parameterize instead +12. **Ignoring cleanup**: Always clean up resources (loggers, Hazelcast instances, etc.) +13. **Not using Java features**: Avoid unnecessary Java syntax (types, access modifiers, semicolons) in Groovy tests unless required + +## Resources + +- [Spock Framework Documentation](https://spockframework.org/spock/docs/2.3/all_in_one.html) +- [Groovy Documentation](https://groovy-lang.org/documentation.html) +- CPS-NCMP codebase: `cps-ncmp-service/src/test/groovy/` diff --git a/.amazonq/rules/agent-standards.txt b/.amazonq/rules/agent-standards.txt new file mode 100644 index 0000000000..3ba3eda4ad --- /dev/null +++ b/.amazonq/rules/agent-standards.txt @@ -0,0 +1 @@ +Follow all AI agent development standards and guidelines documented in /AGENTS.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..cf112df0e4 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1 @@ +Follow the rules in AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..8f896c278b --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,11 @@ +# Global Project Rules +- Language: Java 17 +- Build: Maven +- Framework: Springboot 3.5+ + +## Specialized Skills +The following specialized playbooks are available in `.agents/skills/`: +- **copyright-manager**: Follow this for all file headers. +- **java-quality**: Follow this for java and test development. +- **test-quality**: Follow this for test development. + diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..cf112df0e4 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +Follow the rules in AGENTS.md -- 2.16.6