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

Disable highlight & fix some other tests. #59

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Conversation

chrismaddern
Copy link
Contributor

@chrismaddern chrismaddern commented Oct 1, 2024

PR Type

tests, enhancement


Description

  • Disabled the supportsUrl and supportsContract methods in the HighlightIngestor class by returning false and commented out the existing logic.
  • Removed the symbol field from the GraphQL query in offchain-metadata.ts.
  • Updated the foundation.test.ts to expect priceWei to be '0' due to changes in the API response.
  • Skipped the highlight test suite in highlight.test.ts.
  • Reformatted and updated the rarible.test.ts for consistency and clarity.
  • Updated the README to reflect the "Failing" status of the Base platform in the Mobile Minting section.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Disable URL and contract support in HighlightIngestor       

src/ingestors/highlight/index.ts

  • Disabled the supportsUrl and supportsContract methods by returning
    false.
  • Commented out the existing logic in these methods.
  • +26/-24 
    offchain-metadata.ts
    Remove symbol field from GraphQL query                                     

    src/ingestors/highlight/offchain-metadata.ts

    • Removed the symbol field from the GraphQL query.
    +0/-1     
    Tests
    foundation.test.ts
    Update foundation test for priceWei expectation                   

    test/ingestors/foundation.test.ts

    • Updated test to expect priceWei to be '0' due to API changes.
    +3/-1     
    highlight.test.ts
    Skip highlight test suite                                                               

    test/ingestors/highlight.test.ts

    • Skipped the 'highlight' test suite.
    +1/-1     
    rarible.test.ts
    Reformat and update Rarible test suite                                     

    test/ingestors/rarible.test.ts

  • Reformatted code for consistency.
  • Updated test descriptions and expectations.
  • +183/-169
    Documentation
    README.md
    Update Base platform status to Failing                                     

    README.md

  • Updated status of Base platform to "Failing" in the Mobile Minting
    section.
  • +3/-3     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-merge-pro qodo-merge-pro bot added enhancement New feature or request tests labels Oct 1, 2024
    Copy link

    qodo-merge-pro bot commented Oct 1, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Functionality Removal
    The supportsUrl and supportsContract methods have been disabled by returning false and commenting out the existing logic. This may impact the functionality of the HighlightIngestor class.

    Skipped Tests
    The entire 'highlight' test suite has been skipped. This may lead to reduced test coverage for the HighlightIngestor functionality.

    Test Expectation Change
    The test expectation for priceWei has been changed from a specific value to '0'. This change should be verified to ensure it aligns with the intended behavior of the API.

    Copy link

    qodo-merge-pro bot commented Oct 1, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Testing
    Update tests to reflect the current disabled state of Highlight functionality

    Instead of skipping the entire 'highlight' test suite, consider updating the tests
    to reflect the new behavior of the disabled Highlight functionality. This ensures
    that the current state is properly tested and prevents regression if the
    functionality is re-enabled in the future.

    test/ingestors/highlight.test.ts [10]

    -describe.skip('highlight', function () {
    +describe('highlight', function () {
    +  it('should return false for supportsUrl', async function () {
    +    const ingestor = new HighlightIngestor();
    +    const result = await ingestor.supportsUrl(resources, 'https://example.com');
    +    expect(result).to.be.false;
    +  });
     
    +  it('should return false for supportsContract', async function () {
    +    const ingestor = new HighlightIngestor();
    +    const result = await ingestor.supportsContract(resources, { chainId: 1, contractAddress: '0x123' });
    +    expect(result).to.be.false;
    +  });
    +
    +  // Add more tests for other methods as needed
    +});
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Updating tests to reflect the current state ensures that the code is properly tested and helps prevent regressions, which is crucial for maintaining code quality.

    8
    Enhancement
    Implement a feature flag for easier management of the Highlight functionality

    Instead of completely disabling the supportsUrl and supportsContract methods,
    consider implementing a feature flag or configuration option to toggle this
    functionality. This would allow for easier re-enabling of the feature in the future
    if needed.

    src/ingestors/highlight/index.ts [17-19]

     async supportsUrl(resources: MintIngestorResources, url: string): Promise<boolean> {
    -  return false;
    +  if (!this.isHighlightEnabled()) {
    +    return false;
    +  }
    +  // Original implementation here
     }
     
    +private isHighlightEnabled(): boolean {
    +  // Implement logic to check if Highlight is enabled
    +  return false; // Currently disabled
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Introducing a feature flag provides flexibility for future changes and easier toggling of functionality, which enhances maintainability and adaptability of the code.

    7
    Maintainability
    Add logging to indicate why Highlight functionality is disabled

    Consider logging the reason for disabling the Highlight functionality. This can help
    with debugging and understanding why certain features are not working in production.

    src/ingestors/highlight/index.ts [17-19]

     async supportsUrl(resources: MintIngestorResources, url: string): Promise<boolean> {
    +  console.log('Highlight functionality is currently disabled');
       return false;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding logging can aid in debugging and provide clarity on why certain features are disabled, which is beneficial for maintainability.

    6
    Consistency
    Ensure consistency across all methods affected by the Highlight disabling

    The createMintForContract method is not shown in the diff, but it likely needs to be
    updated to reflect the changes made to supportsUrl and supportsContract. Ensure that
    this method is consistent with the new behavior.

    src/ingestors/highlight/index.ts [36-46]

     async supportsContract(resources: MintIngestorResources, contractOptions: MintContractOptions): Promise<boolean> {
       return false;
     }
     
    +async createMintForContract(resources: MintIngestorResources, contractOptions: MintContractOptions): Promise<MintTemplate> {
    +  throw new Error('Highlight functionality is currently disabled');
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While the suggestion to update createMintForContract for consistency is logical, the method's implementation is not shown in the diff, making it speculative.

    5

    💡 Need additional feedback ? start a PR chat

    @chrismaddern chrismaddern merged commit 182bb7a into main Oct 1, 2024
    1 check passed
    @chrismaddern chrismaddern deleted the chris/fix-tests branch October 1, 2024 22:26
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant