Skip to content

Use latest release from sqlcmd #229

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

zijchen
Copy link
Contributor

@zijchen zijchen commented Jun 13, 2024

@zijchen zijchen temporarily deployed to Automation test June 13, 2024 19:47 — with GitHub Actions Inactive
@zijchen zijchen temporarily deployed to Automation test June 13, 2024 19:47 — with GitHub Actions Inactive
@zijchen zijchen temporarily deployed to Automation test June 13, 2024 23:56 — with GitHub Actions Inactive
@zijchen zijchen temporarily deployed to Automation test June 13, 2024 23:56 — with GitHub Actions Inactive
@zijchen
Copy link
Contributor Author

zijchen commented Jun 14, 2024

Should we introduce a new sqlcmd-version input?

@zijchen zijchen marked this pull request as ready for review June 14, 2024 17:02
@dzsquared
Copy link
Collaborator

Should we introduce a new sqlcmd-version input?
On a similar topic - I think we want to have an implementation in the future that creates a code path where sqlcmd isn't downloaded unless its missing.
At the very least, this will save time in the runner. This also sets environments up to have reproducible sets of software.

* @returns The version number or undefined if the version could not be parsed or HTTP request failed.
*/
public static async extractVersionFromLatestRelease(releaseUrl: string): Promise<string | undefined> {
const http = new HttpClient('Azure/sql-action', undefined, { allowRedirects: false });
Copy link
Contributor

@chlafreniere chlafreniere Jun 17, 2024

Choose a reason for hiding this comment

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

Do we have access to user token (or some sort of system token) from sql-action here? Since this is unauthenticated, it will be much more rate-limited.

If we do, may be easier just to use octokit library instead of httpclient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it doesn't appear there's an easy way to get the token, we'd have to introduce a new input so users can pass in GITHUB_TOKEN to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what the rate limiting will be here - since we're not using a REST API necessarily, we're checking the redirect.

@zijchen zijchen requested a review from llali June 26, 2024 19:32
Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

Review of TypeScript Test Code for Setup.ts

Summary:

The code snippet tests the setupSqlcmd function in TypeScript, ensuring correct setup of SQL command-line tools (sqlcmd). It utilizes Jest for mocking and spying on methods from HttpClient and tc (Tool Cache).

Feedback and Improvements:

  1. Test Function Naming:

    • Ensure descriptive names for test functions. For example, it('sets up sqlcmd correctly') clearly describes the purpose of the test.
  2. Spy Usage:

    • Good use of jest.spyOn to mock HttpClient.prototype.head and various methods from tc for controlled testing environments.
  3. Refactoring Suggestions:

    • Consider refactoring to improve test coverage or to handle additional edge cases specific to setupSqlcmd.
    • Use beforeEach or beforeAll hooks to set up common test conditions, reducing redundancy in test setup.
  4. Conditional Testing:

    • Ensure that conditional testing (if (process.platform === 'win32')) is robust and handles all expected scenarios.

Example Improvements:

describe('Setup.ts tests', () => {
    beforeEach(() => {
        jest.clearAllMocks(); // Reset all spies before each test
    });

    it('sets up sqlcmd correctly', async () => {
        const extractVersionSpy = jest.spyOn(Setup, 'extractVersionFromLatestRelease').mockResolvedValue('1.1.1');
        const cacheLookupSpy = jest.spyOn(tc, 'find').mockReturnValue('');
        const downloadToolSpy = jest.spyOn(tc, 'downloadTool').mockResolvedValue('');
        const extractTarSpy = jest.spyOn(tc, 'extractTar').mockResolvedValue('');

        await Setup.setupSqlcmd();

        expect(extractVersionSpy).toHaveBeenCalled();
        expect(cacheLookupSpy).toHaveBeenCalled();
        expect(downloadToolSpy).toHaveBeenCalled();
        
        if (process.platform === 'win32') {
            // Add platform-specific expectations or tests
        }
    });
});

Conclusion:

The provided test setup effectively verifies the setupSqlcmd function's behavior, utilizing Jest spies to mock dependencies and assert expected function calls. Consider the outlined improvements to enhance code readability, maintainability, and test coverage.

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.

4 participants