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

Fixed should be stale condition #304

Merged
merged 2 commits into from
Feb 5, 2021
Merged

Fixed should be stale condition #304

merged 2 commits into from
Feb 5, 2021

Conversation

nijel
Copy link
Contributor

@nijel nijel commented Feb 4, 2021

When different stale period is used for issues and pull requests, this code uses wrong one.

This was probably missed in #224

Fixes #299

This is not tested at all, but I hope the fix is correct.

When different stale period is used for issues and pull requests, this code uses wrong one.

This was probably missed in actions#224

Fixes actions#299
nijel added a commit to WeblateOrg/meta that referenced this pull request Feb 4, 2021
There seems to be a bug in current actions/stale and it uses the default
value for checking actual staleness of an issue. So use the lower value
there. This will make marking PRs as stale earlier, but we don't have
any stale right now. Hopefully it will get properly fixed in the action.

See actions/stale#304
See actions/stale#299
@nijel nijel mentioned this pull request Feb 4, 2021
Copy link
Contributor

@C0ZEN C0ZEN left a comment

Choose a reason for hiding this comment

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

@C0ZEN
Copy link
Contributor

C0ZEN commented Feb 4, 2021

@nijel thanks for the fix.
This is strange because I added some tests (maybe not covering all options then?).
Could be still good to have at least one?

@nijel
Copy link
Contributor Author

nijel commented Feb 4, 2021

Yes, it would be good to have this covered by the test, but I can't write them sorry (my typescript knowledge is exactly zero).

@C0ZEN
Copy link
Contributor

C0ZEN commented Feb 4, 2021

@nijel ok then I will add some coverage tonight (CET)

@C0ZEN
Copy link
Contributor

C0ZEN commented Feb 4, 2021

@nijel
This is covering this issue:

test('processing an issue opened since 2 days and with the option "daysBeforeIssueStale" at 3 will not make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforeIssueStale: 3
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'An issue with no label',
      issueDate.toDateString(),
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(0);
  expect(processor.closedIssues.length).toEqual(0);
});

test('processing an issue opened since 2 days and with the option "daysBeforeIssueStale" at 2 will make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforeIssueStale: 2
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'An issue with no label',
      issueDate.toDateString(),
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(1);
  expect(processor.closedIssues.length).toEqual(0);
});

test('processing an issue opened since 2 days and with the option "daysBeforeIssueStale" at 1 will make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforeIssueStale: 1
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'An issue with no label',
      issueDate.toDateString(),
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(1);
  expect(processor.closedIssues.length).toEqual(0);
});

test('processing a pull request opened since 2 days and with the option "daysBeforePrStale" at 3 will not make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforePrStale: 3
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'A pull request with no label',
      issueDate.toDateString(),
      issueDate.toDateString(),
      true
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(0);
  expect(processor.closedIssues.length).toEqual(0);
});

test('processing a pull request opened since 2 days and with the option "daysBeforePrStale" at 2 will make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforePrStale: 2
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'A pull request with no label',
      issueDate.toDateString(),
      issueDate.toDateString(),
      true
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(1);
  expect(processor.closedIssues.length).toEqual(0);
});

test('processing a pull request opened since 2 days and with the option "daysBeforePrStale" at 1 will make it stale', async () => {
  expect.assertions(2);
  const opts: IssueProcessorOptions = {
    ...DefaultProcessorOptions,
    daysBeforeStale: 10,
    daysBeforePrStale: 1
  };
  let issueDate = new Date();
  issueDate.setDate(issueDate.getDate() - 2);
  const TestIssueList: Issue[] = [
    generateIssue(
      opts,
      1,
      'A pull request with no label',
      issueDate.toDateString(),
      issueDate.toDateString(),
      true
    )
  ];
  const processor = new IssueProcessor(
    opts,
    async () => 'abot',
    async p => (p == 1 ? TestIssueList : []),
    async (num, dt) => [],
    async (issue, label) => new Date().toDateString()
  );
  
  // process our fake issue list
  await processor.processIssues(1);
  
  expect(processor.staleIssues.length).toEqual(1);
  expect(processor.closedIssues.length).toEqual(0);
});

You can add a commit in your PR with this or eventually you could give me the right to push on your fork.

@nijel
Copy link
Contributor Author

nijel commented Feb 4, 2021

Cool, I've added them.

@C0ZEN
Copy link
Contributor

C0ZEN commented Feb 4, 2021

@hross hello. I made a mistake with the new options regarding the stale days for an issue or a pull request. The tests are confirming that this PR will make these options useful. If you can take the time to merge and release it it would be very nice. Thanks!

@hross hross merged commit 7164109 into actions:main Feb 5, 2021
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.

Stale label not being added
3 participants