Skip to content

Conversation

@Vandita2020
Copy link
Contributor

@Vandita2020 Vandita2020 commented Mar 4, 2025

Problem

Adding metrics and unit test for Serverless Land Project

Solution

  • CreateWizard
    :) Path
    ✔ creates project with Python runtime and CDK (725ms)
    Error Handling
    ✔ handles empty project name (521ms)

  • MetadataManager
    getInstance
    ✔ should create a new instance when none exists
    ✔ should return the same instance when called multiple times
    initialize
    ✔ should initialize metadata manager and return instance
    getMetadataPath
    ✔ should return correct metadata path
    getPatterns
    ✔ handles different pattern data types
    getRuntimes
    ✔ returns empty array when pattern not found
    ✔ returns unique runtimes for valid pattern
    getUrl
    ✔ returns empty string when pattern not found
    ✔ returns correct URL for valid pattern
    getIacOptions
    ✔ returns empty array when pattern not found
    ✔ returns unique IAC options for valid pattern
    getAssetName
    ✔ returns empty string when pattern not found
    ✔ returns correct asset name for matching implementation
    ✔ returns empty string when no matching implementation found

  • createNewServerlessLandProject
    ✔ should complete project creation successfully
    ✔ should handle wizard cancellation
    downloadPatternCode
    ✔ successfully downloads pattern code
    ✔ handles download failure
    openReadmeFile
    ✔ successfully opens README file
    ✔ handles missing README file
    ✔ handles error with opening README file
    getProjectUri
    ✔ returns Uri when file exists
    ✔ handles missing project directory


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Mar 4, 2025

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@Vandita2020 Vandita2020 marked this pull request as ready for review March 4, 2025 21:57
@Vandita2020 Vandita2020 requested a review from a team as a code owner March 4, 2025 21:57
Comment on lines 68 to 69
sandbox.assert.calledOnce(wizardStub)
sandbox.assert.calledOnce(metadataStub)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you using stubs? check actual state.

please read the codebase guidelines.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are integ tests https://github.com/aws/aws-toolkit-vscode/pull/6711/files checking actual state. I assume the reasoning was to stub in the unit tests so that we wouldn't be creating new projects for tests too frequently, @Vandita2020 correct me if I'm wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @seshubaws's explanation is correct here, for unit tests I am using stubs and for integration tests we're checking actual state

Copy link
Contributor

Choose a reason for hiding this comment

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

From a quick look at MetadataManager, I wonder if we could avoid stubs. Filesystem calls are not expensive for tests and you can just read the actual metadata.json file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual metadata.json file may change with time and will get updated. I didn't want tests to fail upon any addition/deletion of pattern in the main metadata.json file

Copy link
Contributor

Choose a reason for hiding this comment

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

What if you just stubbed getMetadataPath to point to a custom test file. Then you wouldn't need to stub anything else and it tests all the real code

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't want tests to fail upon any addition/deletion of pattern in the main metadata.json file

But that is a useful thing to assert. If the content changes, then the test should change.

try {
await getPattern(serverlessLandOwner, serverlessLandRepo, fullAssetName, location, true)
telemetry.record({
action: fullAssetName + config.iac + config.runtime,
Copy link
Contributor

Choose a reason for hiding this comment

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

just making sure, is fullAssetName the name of the template or the name the cx provides?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the way we're creating action. If we want it to be a string, there should be spaces in between, or some separator. But more importantly, this does not currently read as an action. And do we not need a source field to know/sort where this is coming from?

@Vandita2020 Vandita2020 force-pushed the external-metrics branch 5 times, most recently from d3f6cff to 3921f72 Compare March 5, 2025 09:07

mockMetadataManager = sandbox.createStubInstance(MetadataManager)
mockMetadataManager.getAssetName.returns('test-asset-name')
sandbox.stub(MetadataManager, 'getInstance').returns(mockMetadataManager as unknown as MetadataManager)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have time to go through all the details. But this feels like we can reduce the amount of stubs needed, just stubbing lower level things like network calls, and then all real code can execute.

Not blocking for this PR due to time constraints, but please keep in mind when designing for testability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll cover the changes in a follow up PR

import * as downloadPattern from '../../../../shared/utilities/downloadPatterns'
import * as wizardModule from '../../../../awsService/appBuilder/serverlessLand/wizard'

describe('createNewServerlessLandProject', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You can deduplicate code by moving all these describe() in to a nested describe() and then in the top level you create the sandbox once

@Vandita2020 Vandita2020 force-pushed the external-metrics branch 2 times, most recently from 0c772eb to 1b44543 Compare March 5, 2025 18:41
Vandita2020 and others added 17 commits March 5, 2025 11:35
## Problem
Adding entry points and Quick Pick structure for Serverless Land
integration project. The attached images demonstrate the functionality.
The `metadata.json` file is currently a sample, but it will be modified
to include the exact list of patterns.

## Solution
### **EntryPoints**
From AWS Application Builder
<img width="698" alt="Screenshot 2025-02-16 at 7 33 29 PM"
src="https://github.com/user-attachments/assets/1b0b563e-e6f3-414e-ad7d-b895c172b5f3"
/>

From Command Palette
<img width="920" alt="Screenshot 2025-02-16 at 5 10 37 PM"
src="https://github.com/user-attachments/assets/773c2727-344c-4100-8ee5-09efb52809f7"
/>

From Lambda in AWS Explorer
<img width="481" alt="Screenshot 2025-02-18 at 3 22 54 PM"
src="https://github.com/user-attachments/assets/b4b9a9f8-1e34-44ed-8b6d-0aeaac03e7a0"
/>

From the link in Getting Started Walkthrough
<img width="669" alt="Screenshot 2025-02-18 at 3 36 42 PM"
src="https://github.com/user-attachments/assets/f57a3b20-41df-4845-bbb8-c253bf8db592"
/>

### **Quick Pick**

Pattern selection
<img width="893" alt="Screenshot 2025-02-16 at 7 28 25 PM"
src="https://github.com/user-attachments/assets/1844dd2e-f996-4d54-a9f2-bae9af4eee84"
/>

Runtime selection
<img width="917" alt="Screenshot 2025-02-16 at 7 28 33 PM"
src="https://github.com/user-attachments/assets/73532262-70b1-412a-92e3-7ad12075ecb1"
/>

IaC selection
<img width="907" alt="Screenshot 2025-02-16 at 7 28 39 PM"
src="https://github.com/user-attachments/assets/92c093ce-8e0f-42b5-91f9-f0672dbce9f9"
/>

Project location selection
<img width="921" alt="Screenshot 2025-02-16 at 5 12 04 PM"
src="https://github.com/user-attachments/assets/c24b1b99-834b-4e1a-b34a-062ad927bafa"
/>

Project Name
<img width="909" alt="Screenshot 2025-02-16 at 5 12 19 PM"
src="https://github.com/user-attachments/assets/423165bb-ad59-4c15-b6da-582a44de8753"
/>

---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Co-authored-by: Vandita Patidar <[email protected]>
## Problem
This pull request introduces the functionality to allow users to
download the selected pattern from the Serverless Land QuickPick flow
into a directory of their choice.

## Solution
The key changes made are as follows:
1. The `metadata.json` file was updated to accommodate the addition of
an `asset_name` field for each IaC and Runtime pair.
2. Consequently, the way the metadata is extracted in the
`metadataManager.ts` file had to be modified to account for these
changes in the `metadata.json` file.
3. To enable users to navigate back to the previous QuickPick options, a
switch functionality was implemented in the `wizard.ts` file. This helps
maintain the state of the current QuickPick and directs the user to the
previous QuickPick.
---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Co-authored-by: Vandita Patidar <[email protected]>
## Problem
This PR adds the functionality to open pattern in serverless-land
website. It addresses an issue with the `metadata.json` file path for
the extension by specifying the path after the build step in the
`copyFile.ts` file.

## Solution
Changes:
1. Removed the webview functionality and instead open the browser to
preview the patterns.
2. Removed the Github button.
3. Added a link at the bottom of pattern quick pick to explore the
[serverless land
website](https://serverlessland.com/patterns?services=lambda).
4. Ensured the proper path for the metadata file is accessible and it
gets executed properly after the build.
5. Added another entry point at the AWS Explorer high-level, accessible
when clicking the three-dot menu.
@nkomonen-amazon
Copy link
Contributor

/retryBuilds

@nkomonen-amazon nkomonen-amazon merged commit ca46ed3 into aws:feature/serverlessland Mar 5, 2025
16 of 17 checks passed
nkomonen-amazon pushed a commit to nkomonen-amazon/aws-toolkit-vscode that referenced this pull request Mar 5, 2025
## Problem
Adding metrics and unit test for Serverless Land Project

## Solution

- CreateWizard
    :) Path
      ✔ creates project with Python runtime and CDK (725ms)
    Error Handling
      ✔ handles empty project name (521ms)

- MetadataManager
    getInstance
      ✔ should create a new instance when none exists
      ✔ should return the same instance when called multiple times
    initialize
      ✔ should initialize metadata manager and return instance
    getMetadataPath
      ✔ should return correct metadata path
    getPatterns
      ✔ handles different pattern data types
    getRuntimes
      ✔ returns empty array when pattern not found
      ✔ returns unique runtimes for valid pattern
    getUrl
      ✔ returns empty string when pattern not found
      ✔ returns correct URL for valid pattern
    getIacOptions
      ✔ returns empty array when pattern not found
      ✔ returns unique IAC options for valid pattern
    getAssetName
      ✔ returns empty string when pattern not found
      ✔ returns correct asset name for matching implementation
      ✔ returns empty string when no matching implementation found

-   createNewServerlessLandProject
    ✔ should complete project creation successfully
    ✔ should handle wizard cancellation
  downloadPatternCode
    ✔ successfully downloads pattern code
    ✔ handles download failure
  openReadmeFile
    ✔ successfully opens README file
    ✔ handles missing README file
    ✔ handles error with opening README file
  getProjectUri
    ✔ returns Uri when file exists
    ✔ handles missing project directory

---

- Treat all work as PUBLIC. Private `feature/x` branches will not be
squash-merged at release time.
- Your code changes must meet the guidelines in
[CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines).
- License: I confirm that my contribution is made under the terms of the
Apache 2.0 license.

---------

Co-authored-by: Vandita Patidar <[email protected]>

public getMetadataPath(): string {
return globals.context.asAbsolutePath('dist/src/serverlessLand/metadata.json')
return globals.context.asAbsolutePath(path.join('dist', 'src', 'serverlessLand', 'metadata.json'))
Copy link
Contributor

@justinmk3 justinmk3 Mar 6, 2025

Choose a reason for hiding this comment

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

? Why is join() needed here? "/" slashes work just fine on Windows.

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.

5 participants